allenai/pdf-component-library

Single PDF rendering component

Closed this issue · 3 comments

A single component that encapsulates Document Wrapper, outline, Page components etc.
This might also fix the double scroll issue.

Some problems we have with current structure:

  • Developers who use this library have to define <div class="reader__page-list"> and render all pdf pages 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.
  • scroll.ts finds the <div> that has its class attribute set to reader__page-list to scroll. If there are some other
    that use the same class name, the scroll function would fail.

Proposal:

  • Create a new component that encapsulates <div class="reader__page-list> in ui/library, so the library users can render all pdf pages simply by using this component

Benefits:

  • Easier and more intuitive for library users to render pdf pages
  • scroll.ts would target on the component to scroll instead of using document.getElementsByClassName to get the scrollable target. This will avoid failure when other
    use the same class name as the scroll target.
  • Better code structure to maintain and use

Works to do:

  • A new React component (planning to call it PageList.tsx) that renders all pdf pages in ui/library. It will basically encapsulate below lines
    <div className="reader__page-list" ref={pdfScrollableRef}>
    {Array.from({ length: numPages }).map((_, i) => (
    <PageWrapper key={i} pageIndex={i}>
    <Overlay>
    <HighlightOverlayDemo pageIndex={i} />
    <TextHighlightDemo pageIndex={i} />
    <ScrollToDemo pageIndex={i} />
    <CitationsDemo
    annotations={annotations}
    pageIndex={i}
    parentRef={pdfScrollableRef}
    />
    </Overlay>
    </PageWrapper>
    ))}
    </div>

    (Also since pdf data is available on s2airs, and the citation data, scroll function, highlight function are available, I am thinking to drop the demo components under <Overlay>)
  • The component will have a field to receive citation data in prop
  • Update the functions in scroll.ts to let them accept a target element to scroll instead of querying it with document.getElementsByClassName
  • Update unit tests
  • Update demo app and S2 code

Blocked until we fix the double scroll bar.
The PR creates a component to render all pdf pages but it also gonna be the scroll target for scroll functions to work on. However, we are keeping the outer scroll bar of the reader where the scroll target gonna be an element on S2 site, it conflicts with the PR. So, I’ll leave the PR open until Huy solves the double scroll bar, then we revisit it. Also, we may need to change the scroll functions in pdf-component-library if scroll target gonna be an element on S2 site.

No longer needed, we now have scroll context.