PDF Component Library loose ends
Closed this issue · 0 comments
carolinepaulic commented
Legitimate tech debt that needs to be fixed. At this point it's a list of pending good to haves.
DONE :)
Bonus/nice to haves
- Unit tests for
Reader
. Components have tests, missing an integration test for the Reader - 3 days - Unit tests for
rotate.ts
- 3 days - Add optional callback props to PDF load handlers #34 (comment). Might be nice to have a callback to handle onError events. - 3 day
- DocumentWrapper- use more specific props #34 (comment)
- Look into use of FunctionComponent vs StaticFunctionalComponent
- Establish import sorting rules for JS files (#17 (comment))
- Add PR template using the following as an example- https://github.com/allenai/scholar/blob/main/.github/pull_request_template.md
-
PageRotation
: move default/Rotate0 case to top, add comments showing rotation visually - Update
errorUtils.ts
getErrorMessage
parameter type and remove line that disables linting - Update JSON formatter to leave EOF newline on save
- Hot reload for demo app
- Answer performance related questions based on metrics created in #30640
- How does placement of setting outline context variable affect performance? Is there a difference if we set the outline in
DocumentWrapper.tsx
onPdfLoadSuccess
callback vs setting inOutline.tsx
- Does O(n^3) bounding box rendering time complexity significantly affect performance?
- How does placement of setting outline context variable affect performance? Is there a difference if we set the outline in
Done
- Unit tests for new context providers
- Relevant comments from old PR: #17 (comment), #17 (comment)
- New PR: #19
- Log message when context setters aren't instantiated
- Relevant comment from old PR: #17 (comment)
- New PR: #20
- Zoom control: replace child component props with context vars where possible
- PR: #21
- Look into
Header
'
vs'
- Solution no longer needed because this line was removed from
Header
altogether, but I did some research and learned more about when escaping chars is and isn't needed. In this case I think it would have been ok as'
. - Drawer/Outline: move into own component and replace child component props with context vars where possible (contains bugfix for outline not opening)
- PR: #24
- Header: replace child component props with context vars where possible
- PR: #25
- PageWrapper: replace child component props with context vars where possible
- PR: #37
- allenai/scholar#29449
- allenai/scholar#29577
- allenai/scholar#29467
- #17 (comment)
SimpleZoomControl
(#17 (comment)),Header
(#17 (comment))- Possible solution: https://jotai.pmnd.rs/
- Pull page width computation functions from
Overlay
andPageWrapper
into shared util - allenai/scholar#29578
- allenai/scholar#29696