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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

React-Virtualized PF4 tables #2011

Merged
merged 5 commits into from Jun 7, 2019
Merged

React-Virtualized PF4 tables #2011

merged 5 commits into from Jun 7, 2019

Conversation

@priley86
Copy link
Member

priley86 commented May 15, 2019

What:
Refactors #1250
Fixes #1229 #1628

Downstream PR:
openshift/console#1465

Q&A 馃嵖 馃嵃

What's wrong with reactabular-virtualized?
As noted in #1250 - dynamic row heights cause a problem when scrolling really long lists. In order to correctly calculate scroll offset, we need to measure all rows. We can't do this when the user drag scrolls an extremely long list. If using fixed heights though, this solution works perfectly fine.

Why bring in react-virtualized Grid source?
By default, Grid renders div's instead of tbody and tr's here. This negates all PF4 table styling for table body's and table row's.

Why am I not using @patternfly/react-table TableBody?
React-Virtualized uses absolute row positions to resolve dynamic row height issue mentioned above. This breaks the default layout for PF4 table rows. However, we can still emulate PF4 styling by assigning tr's with display: table; table-layout: fixed. As such, I am defining a new API surface for table body and table row which works with react-virtualized, but suggests the common use of Table/TableHeader for defining table headers.

Why am I going through this trouble?
OpenShift currently has very specific requests about emulating this virtualization behavior as well as retaining the existing mobile behavior. However, integrating @patternfly/react-table now will open the door for using our React tables out-of-the-box when virtualization is not needed. It should also work w/ vanilla PF4 if we were to choose pagination or infinite scroll for some other visualizations (rather than virtual scroll) giving us a more common API surface.

Running Demo:
https://2011-pr-patternfly-react-patternfly.surge.sh/patternfly-4/virtual%20scroll/virtualized/

A11Y notes:
The DOM structure allowed by react-virtualized does not fit conventional grid DOM hierarchy b/c it relies on having a nested rowgroup element that is scrollable. To resolve this issue, I am using the following fix for fixing incorrect DOM structures with roles/aria. I've tested it with VoiceOver on Mac and it seems to work quite well now. It also passes aXe audits in Chrome.
https://www.html5accessibility.com/tests/aria-table-fix.html

Additional A11y articles on this by Steve Faulkner:
https://developer.paciellogroup.com/blog/2014/10/notes-on-fixing-incorrect-table-structure-using-aria/
https://developer.paciellogroup.com/blog/2018/03/short-note-on-what-css-display-properties-do-to-table-semantics/
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_presentation_role

remaining todos:

  • tests
  • convert flow js to typescript where necessary
  • ensure aria-label can be passed as null if role === 'presentation'. Presentational table elements do not get the same aria attributes.
  • prototype expand/collapse. Expanded rows will be rendered as separate tr's Determined this is not needed downstream at the moment. Can be added in a future PR.

cc: @rhamilto @suomiy

Additional issues:

@priley86 priley86 requested a review from jeff-phillips-18 May 15, 2019
@priley86 priley86 mentioned this pull request May 15, 2019
76 of 89 tasks complete
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented May 15, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 15, 2019

Codecov Report

鉂楋笍 No coverage uploaded for pull request base (master@5121800). Click here to learn what that means.
The diff coverage is 77.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2011   +/-   ##
=========================================
  Coverage          ?   80.65%           
=========================================
  Files             ?      660           
  Lines             ?     8322           
  Branches          ?      628           
=========================================
  Hits              ?     6712           
  Misses            ?     1310           
  Partials          ?      300
Flag Coverage 螖
#patternfly3 85.22% <酶> (?)
#patternfly4 76.15% <77.16%> (?)
#patternflymisc 95.68% <酶> (?)
Impacted Files Coverage 螖
...rnfly-4/react-table/src/components/Table/Header.js 100% <酶> (酶)
...ble/src/components/Table/base/columns-are-equal.js 0% <0%> (酶)
...-4/react-table/src/components/Table/BodyWrapper.js 100% <100%> (酶)
...-4/react-table/src/components/Table/base/header.js 100% <100%> (酶)
...act-table/src/components/Table/base/merge-props.js 100% <100%> (酶)
...e/src/components/Table/base/evaluate-formatters.js 100% <100%> (酶)
...ernfly-4/react-table/src/components/Table/Table.js 86.36% <100%> (酶)
...ly-4/react-table/src/components/Table/base/body.js 52.94% <52.94%> (酶)
.../react-table/src/components/Table/base/body-row.js 57.89% <57.89%> (酶)
...e/src/components/Table/base/evaluate-transforms.js 66.66% <66.66%> (酶)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 5121800...dd3c41c. Read the comment docs.

@priley86 priley86 mentioned this pull request May 15, 2019
6 of 9 tasks complete
@dlabaj dlabaj requested a review from karelhala May 17, 2019
@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented May 17, 2019

@karelhala Can you review this one.

@priley86 priley86 force-pushed the priley86:virtualized branch from c04138c to fa83e87 May 17, 2019
childrenToDisplay
);
}
return React.createElement(scrollContainerComponent, scrollContainerProps, [

This comment has been minimized.

Copy link
@priley86

priley86 May 17, 2019

Author Member

I'll open an upstream PR to react-virtualized for this piece... the consumer should be able to override the default scroll elements rendered.

This comment has been minimized.

Copy link
@priley86

priley86 May 17, 2019

Author Member

this would also add support for the implementation of a virtual scrolling Data List in the future...

This comment has been minimized.

Copy link
@priley86

priley86 May 20, 2019

Author Member

i've opened #1377 upstream in react-virtualized. This would simplify the API consumption there quite a bit if merged.

@priley86 priley86 force-pushed the priley86:virtualized branch from fa83e87 to b0f2de1 May 17, 2019
@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented May 20, 2019

Set a very long string in one of the columns:

image

Seems the height of the row is not growing correctly and it's over writing the next two rows.

Also, notice the column headers (like Workspaces) do not horizontally align with the row fields.

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented May 20, 2019

The row height issue is not relevant as the height is computed based on the data. I changed the value via the inspector which does not cause a recompute. Sorry for any confusion.

@priley86 priley86 mentioned this pull request May 20, 2019
6 of 6 tasks complete
Copy link
Contributor

karelhala left a comment

Foo, this one is really big! Everything looks good, does this mean that we can change react tabular to fit our needs?

@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented May 22, 2019

thanks @karelhala - i still have a couple todo's here. Will try to update the PR again soon.

@priley86 priley86 force-pushed the priley86:virtualized branch from 34b6745 to ed51760 Jun 4, 2019
@dlabaj dlabaj self-assigned this Jun 5, 2019
@priley86 priley86 force-pushed the priley86:virtualized branch 2 times, most recently from 709c3e7 to bf970e4 Jun 5, 2019
@priley86 priley86 force-pushed the priley86:virtualized branch from bf970e4 to 9156534 Jun 5, 2019
priley86 and others added 3 commits Jun 5, 2019
Copy link
Contributor

redallen left a comment

Looks great, thanks!! 馃憤

@priley86 priley86 mentioned this pull request Jun 6, 2019
2 of 4 tasks complete
@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Jun 6, 2019

thanks @dlabaj and @redallen !

I would like to begin integration testing this downstream and open this as a prerelease for testing to others. I've filed #2179 for future follow up... let me know if anything else is needed before merge.

@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Jun 6, 2019

cc: @dgutride

@dlabaj
dlabaj approved these changes Jun 7, 2019
@dgutride dgutride merged commit 84122a1 into patternfly:master Jun 7, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.