feat(Table): Add Tree Table variant #5573

nicolethoen · 2021-03-25T15:26:23Z

What: Closes #5016

nicolethoen · 2021-03-25T15:35:14Z

I know there is still some clean up to do around aria attributes, and possibly documentation.
That's what I'll be working on while I wait for more feedback.

patternfly-build · 2021-03-25T16:03:37Z

PF4 preview: https://patternfly-react-pr-5573.surge.sh
A11y report: https://patternfly-react-pr-5573-a11y.surge.sh

nicolethoen · 2021-03-25T18:23:12Z

Im looking into allowing a partially selected checkbox state

nicolethoen · 2021-03-25T20:15:51Z

confirmed - a partially checked checkbox is possible. We could build an example or demo in a future story to show how to wire up the checkboxes to cascade up.

packages/react-table/src/components/Table/Table.tsx

jessiehuff

This looks pretty good, Nicole! 🙂 I tested with VO, and it sounded great. It doesn't quite meet the keyboard interaction expectations that are typically expected for a tree view (arrow key functionality in particular), but I know that we're trying to get this in so perhaps we can open another issue for the keyboard functionality? I detailed some of the keyboard expectations here, and I would expect it to follow these guidelines.

mattnolting

This looks great @nicolethoen!

Two things, can we wrap the primary or title th with pf-c-table__tree-view-title-cell and can we remove pf-m-grid-md (bc that will never really work for tree view).

mattnolting

Excellent work, @nicolethoen! LPTM! 🎉🎉🎉

jschuler

Nice job @nicolethoen looks pretty good! just some minor things here.
Also, i noticed in the demo that when i checked a parent, the children were not automatically checked, i would expect them to be

jschuler · 2021-03-29T13:22:25Z

packages/react-table/src/components/Table/Table.tsx

@@ -243,7 +246,6 @@ export class Table extends React.Component<TableProps, {}> {
          role={role}
          variant={variant}
          borders={borders}
-          className={className}


should this be removed?

I had changed the props a handful of times while working on this, so in the end while cleaning stuff up, I guess I realized that className was being pulled out of props unnecessarily on line 161. I can put it back, but it's now passed when I spread props to the Provider on line 230.

packages/react-table/src/components/Table/TableTypes.ts

packages/react-table/src/components/Table/examples/Table.md

jschuler · 2021-03-29T13:39:27Z

packages/react-table/src/components/Table/examples/Table.md

+              isHidden,
+              level: level,
+              posinset: posinset,
+              setsize: x.children ? x.children.length : 0,


can this be camel cased everywhere? setSize

it could, but the aria attribute is not camel cased (i.e. aria-posinset, aria-setsize), so I figured it was less confusing to format it the same way. I could also just use the actual aria attribute name, but number of quotes i'd need to wrap the attribute name annoyed me.

What do you vote? @tlabaj @kmcfaul?

I think using 'aria-posinest' and 'aria-setsize' with quotes would be the clearest. Or at least having "aria" in the property name to be clear what the prop is intended for ( i.e. ariaPosInset / ariaSetSize )

packages/react-table/src/components/Table/examples/Table.md

packages/react-table/src/components/Table/utils/decorators/treeRow.tsx

packages/react-table/src/components/TableComposable/Td.tsx

packages/react-table/src/components/TableComposable/Tr.tsx

nicolethoen · 2021-03-29T17:15:35Z

I will have opened three follow up issues -

Add the roving tabindex to enhance the keyboard accessibility Table: Tree table - use roving tabindex to update keyboard accessibility #5596
Update the example to demonstrate how the consumer can use group checking, where checking a checkbox will check all children rows, and checking a children row will partially check a parent row Table: Tree table - update checkbox behavior in examples #5597
Allow an icon to be included in each row to make the core examples. Table: Tree table - add icon to tree rows #5598

dlabrecq · 2021-03-29T17:48:10Z

packages/react-table/src/components/Table/examples/Table.md

+    - `posinset` - number representing where in the order this node sits amongst its siblings 
+    - `setsize` - number representing the number of children this node has


I know this is an example, but would prefer to see aria-posinset and aria-setsize here, mainly because nothing indicates these are aria attributes.

kmcfaul · 2021-03-29T19:06:23Z

packages/react-table/src/components/Table/TreeRowWrapper.tsx

+export const TreeRowWrapper: React.FunctionComponent<RowWrapperProps> = ({
+  className,
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  rowProps,


Is rowProps something used in only non-composable tables? Why is it being withheld from spreading to the Tr?

I am not totally sure all the reasons the rowData prop exists. At the very least it keeps track of rowIndex and rowKey for the sake of certain callback functions. But, I know that it's already being withheld in the RowWrapper, and if I don't withhold it in the TreeRowWrapper, it spreads all the way to the <tr> element and causes a console warning.

jessiehuff

Unfortunately the new roles seem to break accessibility for screen readers. 😞 When testing in VO, it reads "Tree Table, table, 4 columns 1 row", I'm able to get it to read each of the column headers, but then it says "end of table" without ever getting to the tree table content itself. I don't think the tree and treeitem roles are working well with the table elements, but that's a theory. I tested in JAWS and NVDA as well. JAWS was at least able to make slightly more of a guess, but it separated the headers and the tree table content so it announced the top part as a table and then only viewed the content as a tree view component (gave no features of a table, for example it never announced the column headers as you're navigating the content, just announces it as "tree view" then reads out all of the text). In NVDA, I had difficulty navigating it. It seemed to kind of work at times, other times it would jump around. When trying to expand sections, it would check the previous section, etc. I would describe the interaction as odd for lack of a better word.

If we want to keep the tests, I'd rather remove the attributes getting flagged and add them back later, but ideally I'd love for axe-core to turn on support for these attributes so we can follow the W3 example. There's already an open issue for it with Axe that Sarah Higley (a big name/thought leader in the a11y community) opened for the same reason we've run into. I can comment on that issue and mention our use case to hopefully get that pushed along so we can add these attributes as well.

nicolethoen · 2021-03-29T20:02:14Z

@jessiehuff
so, the attributes getting flagged by the tests are attached to the styles. For example if it has aria-level='3', the css knows to indent it three times. at this point we cannot remove the problematic attributes. But I can remove the roles i just added to get the tests to pass.

I think our only option for this PR is to try and skip the examples in the testing...
@jschuler could you help me figure out how to modify the script to do that?

packages/react-docs/patternfly-a11y.config.js

mcarrano

This looks great from my perspective. Thanks for all the work on this @nicolethoen !

jessiehuff

This looks awesome, Nicole!!!! 🎉 🏆 Thank you so much for all of your effort on this, you did a phenomenal job. I just tested in VO, NVDA, and JAWS, and all of them work like a dream with this structure. 😄

patternfly-build · 2021-03-30T17:59:03Z

Your changes have been released in:

@patternfly/react-docs@5.17.0
@patternfly/react-inline-edit-extension@4.6.64
@patternfly/react-table@4.24.0
@patternfly/react-virtualized-extension@4.7.0

Thanks for your contribution! 🎉

nicolethoen requested review from jschuler, mattnolting, mcarrano, jessiehuff, tlabaj and redallen March 25, 2021 15:26

nicolethoen requested a review from mturley March 25, 2021 18:24

tlabaj reviewed Mar 26, 2021

View reviewed changes

packages/react-table/src/components/Table/Table.tsx Outdated Show resolved Hide resolved

jessiehuff previously approved these changes Mar 26, 2021

View reviewed changes

mattnolting requested changes Mar 26, 2021

View reviewed changes

nicolethoen dismissed jessiehuff’s stale review via 999ffa8 March 26, 2021 19:24

mattnolting self-requested a review March 26, 2021 20:44

mattnolting previously approved these changes Mar 26, 2021

View reviewed changes

nicolethoen dismissed mattnolting’s stale review via e66ef64 March 26, 2021 21:31

jschuler reviewed Mar 29, 2021

View reviewed changes

nicolethoen added 6 commits March 29, 2021 10:06

feat(Table): Add Tree Table variant

816c4e1

update aria-labels and snapshots

e1efde1

clean up checkbox ids in composable example, and add more documentation

0695d6f

update per PR comments

107966d

update snapshots

f38f71e

updates to fix tests and address PR comments

4f7ab69

nicolethoen force-pushed the modify_tree_view_with_table branch from e66ef64 to 4f7ab69 Compare March 29, 2021 16:44

dlabrecq reviewed Mar 29, 2021

View reviewed changes

kmcfaul reviewed Mar 29, 2021

View reviewed changes

jessiehuff reviewed Mar 29, 2021

View reviewed changes

nicolethoen added 2 commits March 29, 2021 20:39

rename props to add 'aria-' prefix

9421e1c

skip new tree table examples in a11y tests

13dd3bc

jschuler reviewed Mar 30, 2021

View reviewed changes

packages/react-docs/patternfly-a11y.config.js Outdated Show resolved Hide resolved

update a11y config and example documentation with note about axe tests

b07df37

mcarrano approved these changes Mar 30, 2021

View reviewed changes

jessiehuff approved these changes Mar 30, 2021

View reviewed changes

dlabrecq approved these changes Mar 30, 2021

View reviewed changes

jessiehuff mentioned this pull request Mar 30, 2021

grid vs. treegrid allowed row attributes dequelabs/axe-core#2794

Closed

kmcfaul approved these changes Mar 30, 2021

View reviewed changes

redallen merged commit 6577d4f into patternfly:master Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Table): Add Tree Table variant #5573

feat(Table): Add Tree Table variant #5573

nicolethoen commented Mar 25, 2021

nicolethoen commented Mar 25, 2021

patternfly-build commented Mar 25, 2021 •

edited

Loading

nicolethoen commented Mar 25, 2021

nicolethoen commented Mar 25, 2021

jessiehuff left a comment

mattnolting left a comment •

edited

Loading

mattnolting left a comment

jschuler left a comment

jschuler Mar 29, 2021

nicolethoen Mar 29, 2021

jschuler Mar 29, 2021

nicolethoen Mar 29, 2021

kmcfaul Mar 29, 2021

nicolethoen commented Mar 29, 2021

dlabrecq Mar 29, 2021 •

edited

Loading

kmcfaul Mar 29, 2021 •

edited

Loading

nicolethoen Mar 29, 2021

jessiehuff left a comment

nicolethoen commented Mar 29, 2021 •

edited

Loading

mcarrano left a comment

jessiehuff left a comment

patternfly-build commented Mar 30, 2021

		- `posinset` - number representing where in the order this node sits amongst its siblings
		- `setsize` - number representing the number of children this node has

feat(Table): Add Tree Table variant #5573

feat(Table): Add Tree Table variant #5573

Conversation

nicolethoen commented Mar 25, 2021

nicolethoen commented Mar 25, 2021

patternfly-build commented Mar 25, 2021 • edited Loading

nicolethoen commented Mar 25, 2021

nicolethoen commented Mar 25, 2021

jessiehuff left a comment

Choose a reason for hiding this comment

mattnolting left a comment • edited Loading

Choose a reason for hiding this comment

mattnolting left a comment

Choose a reason for hiding this comment

jschuler left a comment

Choose a reason for hiding this comment

jschuler Mar 29, 2021

Choose a reason for hiding this comment

nicolethoen Mar 29, 2021

Choose a reason for hiding this comment

jschuler Mar 29, 2021

Choose a reason for hiding this comment

nicolethoen Mar 29, 2021

Choose a reason for hiding this comment

kmcfaul Mar 29, 2021

Choose a reason for hiding this comment

nicolethoen commented Mar 29, 2021

dlabrecq Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

kmcfaul Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

nicolethoen Mar 29, 2021

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

nicolethoen commented Mar 29, 2021 • edited Loading

mcarrano left a comment

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

patternfly-build commented Mar 30, 2021

patternfly-build commented Mar 25, 2021 •

edited

Loading

mattnolting left a comment •

edited

Loading

dlabrecq Mar 29, 2021 •

edited

Loading

kmcfaul Mar 29, 2021 •

edited

Loading

nicolethoen commented Mar 29, 2021 •

edited

Loading