Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

W.I.P. React virtualized extension #1250

Closed

Conversation

priley86
Copy link
Member

@priley86 priley86 commented Jan 25, 2019

What:
A very very rough W.I.P. to start some initial discussions about a "virtualized" PF4 table.

fixes #1229

Some things I've attempted so far:

  • pull in some of the Reactabular virtualized components and wrap our existing table Body, BodyWrapper, and RowWrapper components with virtualized extensions (accepting those as props in our current Table)
  • Render a simple virtualized table with percent widths. In order to scroll a virtualized table, the author recommends using flexbox for flexible widths with virtually scrolled rows. Needs more discussion and some CSS review, but I've included some sample CSS to tinker with:
    https://reactabular.js.org/#/features/virtualization?a=using-relative-column-widths
  • Render a sortable virtualized table. It's important to keep in mind that virtualized rows are dependent on a scroll position (in order to performantly reuse them). For now, I have made the simple assumption that after virtualized sorting occurs to scrollTop, but we can certainly account for an existing scroll position if desired. The scrollTo method should allow the consumer to scroll to any desired row programmatically.

Still working on:

  • provide an AutoSizer (similar to react-virtualized Autosizer). These essentially just wrap the table and adjust the height/widths appropriately on browser resize. This should not be needed. PF4 tables already responsively size the row widths and the virtualized table resizes for this.
  • provide an option and an example to override the scroll container. This allows the consumer to define the scrollable surface. Similar to WindowScroller from react-virtualized or simply using the existing functionality in reactabular-virtualized. After the two options above are finished, we can support a very similar API to the one in use today:
<WindowScroller scrollElement={document.getElementById('content-scrollable')}>
        {({height, isScrolling, registerChild, onChildScroll, scrollTop}) =>
            <Table
              caption="Sortable Virtualized Table"
              cells={columns}
              rows={rows}
              bodyWrapper={VirtualizedBodyWrapper}
              rowWrapper={VirtualizedRowWrapper}
              sortBy={sortBy}
              onSort={this.onSort}
            >
              <TableHeader />
              <VirtualizedBody height={height} width={width} onScroll={onScroll} rowKey="id" tableBody={this.tableBody} />
            </Table>}
      </WindowScroller>
  • test out other Table variations and provide examples w/ virtualized scrolling (selectable table, actions table, collapsible table, etc). These should all work similarly to the way they do today. Selectable table and actions table have been implemented. Collapsible is currently out of scope for this because dynamic row heights in conjunction with virtualized rows introduces problems we should probably avoid for now.
  • Continue enhancing the Reactabular API surface with latest React methods and propose those changes upstream in Reactabular. In this PR, I have started these enhancements (using new Context API, use refs more appropriately, start to upgrade the lifecycles methods to React 16.4 plus. Opened 1628 for this.
  • closes PF4 Bring Reactabular in-house #1628 bring reactabular-table source in-house. This is necessary to continue improving the base table used and to remove legacy React warnings caused by the old ref interface there.
  • rebase PF4 React gatsby updates
  • tests
  • implement scrollable data grid a11y as best as possible (Example 3).
  • convert to typescript where possible

Running demo:
http://pf4-table-virtualized.surge.sh/virtual-scroll/virtualized/

Additional issues:

cc: @karelhala @suomiy

@patternfly-build
Copy link
Contributor

PatternFly-React preview: https://1250-pr-patternfly-react-patternfly.surge.sh

@coveralls
Copy link

coveralls commented Jan 25, 2019

Pull Request Test Coverage Report for Build 4209

  • 18 of 18 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 80.159%

Totals Coverage Status
Change from base Build 4199: 0.005%
Covered Lines: 4624
Relevant Lines: 5416

💛 - Coveralls

@priley86 priley86 force-pushed the react-virtualized-extension branch 2 times, most recently from 3a002eb to e0eaef3 Compare January 30, 2019 01:54
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Few nitpicks here and there, but that's nothing too crucial. I think that this is really great example of how to introduce extended logic to our packages.

@priley86

This comment has been minimized.

@priley86 priley86 force-pushed the react-virtualized-extension branch from 8266044 to b0a1a4c Compare March 27, 2019 18:52
@priley86
Copy link
Member Author

priley86 commented Apr 12, 2019

Demo:
http://pf4-table-virtualized.surge.sh/virtual-scroll/windowscroller/

Note: I have left the other examples w/ fixed headers/scrolling tbody, however they could be used in conjunction w/ WindowScroller as well.

cc: @mcoker @jeff-phillips-18

@priley86

This comment has been minimized.

@priley86 priley86 force-pushed the react-virtualized-extension branch from 39c4644 to 98ff900 Compare April 16, 2019 20:30
@priley86

This comment has been minimized.

@priley86
Copy link
Member Author

Adding a few more fixes after testing some downstream...

3ee7704

  • @patternfly/react-table must be installed as a peerDependency. There is currently a problem with transient dependencies and exposing React's Context to consumers. You can expose a new instance of Context if your extension consumes the same context as the root application if placed in dependencies. This is really an issue in React that should probably be added to React docs:
    React.createContext singleton enforcement? facebook/react#13346
    https://github.com/reactjs/reactjs.org/pull/1112/files

  • fix container offset - using the offsetHeight of tbody is not guaranteed to provide the correct offset. This should make it more consistent.

  • fix possible race condition w/ resize listeners on tbody and container, only listen to resize on container if provided.

@priley86 priley86 force-pushed the react-virtualized-extension branch 6 times, most recently from e33f0f1 to 8a21eb7 Compare May 1, 2019 20:11
@priley86 priley86 force-pushed the react-virtualized-extension branch from 4c21f70 to a2c5819 Compare May 10, 2019 17:03
@priley86 priley86 force-pushed the react-virtualized-extension branch from a2c5819 to 18734fe Compare May 12, 2019 23:40
@jeff-phillips-18 jeff-phillips-18 added the PF4 React issues for the PF4 core effort label May 15, 2019
@priley86 priley86 mentioned this pull request May 15, 2019
4 tasks
@priley86
Copy link
Member Author

Closing in favor of #2011

@priley86 priley86 closed this May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge extension PF4 React issues for the PF4 core effort TypeScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PF4 Bring Reactabular in-house PF4: Virtualized Table/Virtualized Extension
8 participants