Add notice for invalid/illegal fonts detected in harvesting (BL-15003)#594
Conversation
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @StephenMcConnel)
src/components/BookDetail/InvalidFontNotice.tsx line 28 at r1 (raw file):
"We cannot fully present this book because it uses one or more fonts that are not known to be free and open-licensed. Therefore, the PDF and source files are all we are allowed to distribute. Please try to replace these fonts with ones that have open licenses:", })} {listOfNames}
Not a big deal, but we shouldn't need the .
If you make changes for the comment above, you might as well remove this, too. Thanks.
src/components/BookDetail/HarvesterProblemNotice.tsx line 24 at r1 (raw file):
return <InvalidFontNotice book={props.book} />; if (props.book.getMissingFontNames().length > 0) return <MissingFontNotice book={props.book} />;
if (
props.book.getMissingFontNames().length > 0 &&
props.book.getInvalidFontNames().length > 0
) {
return (
<div>
<MissingFontNotice book={props.book} />
<InvalidFontNotice book={props.book} />
</div>
);
}
if (props.book.getInvalidFontNames().length > 0)
return <InvalidFontNotice book={props.book} />;
if (props.book.getMissingFontNames().length > 0)
return <MissingFontNotice book={props.book} />;
I believe all this can be simplified to
return (
<>
<MissingFontNotice book={props.book} />
<InvalidFontNotice book={props.book} />
</>
);
(The controls already return null if there are no font names.)
If there's a layout issue you're trying to resolve, can we fix it another way? Or we should at least put a quick comment so the next dev doesn't assume what I did and try to simplfy it.
2fed1ff to
96db4c5
Compare
StephenMcConnel
left a comment
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)
src/components/BookDetail/HarvesterProblemNotice.tsx line 24 at r1 (raw file):
Previously, andrew-polk wrote…
if ( props.book.getMissingFontNames().length > 0 && props.book.getInvalidFontNames().length > 0 ) { return ( <div> <MissingFontNotice book={props.book} /> <InvalidFontNotice book={props.book} /> </div> ); } if (props.book.getInvalidFontNames().length > 0) return <InvalidFontNotice book={props.book} />; if (props.book.getMissingFontNames().length > 0) return <MissingFontNotice book={props.book} />;I believe all this can be simplified to
return ( <> <MissingFontNotice book={props.book} /> <InvalidFontNotice book={props.book} /> </> );(The controls already return null if there are no font names.)
If there's a layout issue you're trying to resolve, can we fix it another way? Or we should at least put a quick comment so the next dev doesn't assume what I did and try to simplfy it.
Done. It does need a check for whether one or both of the font notices has data so that it can show the generic failure message appropriately.
src/components/BookDetail/InvalidFontNotice.tsx line 28 at r1 (raw file):
Previously, andrew-polk wrote…
Not a big deal, but we shouldn't need the
.
If you make changes for the comment above, you might as well remove this, too. Thanks.
Actually, we need something to get a space between the colon at the end of the localized message and the first font name from the list of font names. But a plain space will do.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @StephenMcConnel)
src/components/BookDetail/HarvesterProblemNotice.tsx line 24 at r1 (raw file):
Previously, StephenMcConnel (Steve McConnel) wrote…
Done. It does need a check for whether one or both of the font notices has data so that it can show the generic failure message appropriately.
Thanks. I missed that.
src/components/BookDetail/InvalidFontNotice.tsx line 28 at r1 (raw file):
Previously, StephenMcConnel (Steve McConnel) wrote…
Actually, we need something to get a space between the colon at the end of the localized message and the first font name from the list of font names. But a plain space will do.
I guess I'd forgotten that bit about tsx. Was thinking of html where any whitespace means a space.
96db4c5 to
578d479
Compare
This change is