allenai/pdf-component-library

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 in Outline.tsx
    • Does O(n^3) bounding box rendering time complexity significantly affect performance?

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
  • allenai/scholar#29449
  • allenai/scholar#29577
  • allenai/scholar#29467
  • Pull page width computation functions from Overlay and PageWrapper into shared util
  • allenai/scholar#29578
  • allenai/scholar#29696