search

allenai / pdf-component-library

51 stars 5 forks source link

Single pdf list component #131

Closed yensung closed 2 years ago

yensung commented 2 years ago

Description

https://github.com/allenai/pdf-component-library/issues/126 Developers who use this library have to define <div class="reader__page-list"> and render all pdf pages with <PageWrapper> under the element by themselves. If it is not set properly, they would fail rendering pdf pages. This is not very intuitive and convenient to use. Also, scroll.ts finds the

that has its class attribute set to reader__page-list to scroll. If there are some other elements that has the same class name, the scroll function may fail. This PR addresses above issues by creating a new component <PageList>, so that developers can render all pdf pages simply by using the component. This also avoids scroll function from failure if multiple elements with reader__page-list class name are defined.

Reviewer Instructions

  • Created a new component PageList.tsx that renders <PageWrappers> for pdf pages
  • Functions in scroll.ts no longer find target div to scroll by document.getElementsByClassName. Instead, the scroll target should be passed as a parameter when using those functions
  • There is a scenario where we would like to render elements on specific pages, such as citations. In this case, we can create a mapping between react elements and pages and pass it to pageElementMap (probably there is a better naming) prop of PageList.tsx
  • The scroll target
    is disclosed in DocumentContext, in case for custom elements such as citation popovers to hook on

Testing Plan

Passed all unit tests. Also tested on S2 successfully with the build.