fix(Table) clean up 'Expand collapse all' demo #8988
Conversation
|
Preview: https://patternfly-react-pr-8988.surge.sh A11y report: https://patternfly-react-pr-8988-a11y.surge.sh |
| }; | ||
| }); | ||
| const setServerExpanded = (server, isExpanding) => { | ||
| const otherExpandedServerNames = expandedServerNames.filter((r) => r !== server.name); |
There was a problem hiding this comment.
should have a setAreAllExpanded here (or in an effect that looks at expandedServerNames) as toggling a single thing could affect that
dominik-petrik
force-pushed
the
table-expandCollapseAll-cleanup
branch
from
bf7d11b to
4689149
Compare
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
I'm also seeing the codesandbox throw an error about the onPageResize not being defined for the dashboardwrapper. i'm also seeing the same issue in other demos in the codesandbox, though so that can be addressed in the future.
| @@ -1,28 +1,19 @@ | |||
| import React from 'react'; | |||
| import { Card, Label, PageSection } from '@patternfly/react-core'; | |||
There was a problem hiding this comment.
Can you combine the react-core imports and make sure that Text and TextContent are also added?
| @@ -1,28 +1,19 @@ | |||
| import React from 'react'; | |||
| import { Card, Label, PageSection } from '@patternfly/react-core'; | |||
| import { expandable } from '@patternfly/react-table'; | |||
There was a problem hiding this comment.
This is an unused import of expandable
| const setServerExpanded = (server, isExpanding) => { | ||
| const otherExpandedServerNames = expandedServerNames.filter((r) => r !== server.name); | ||
| setExpandedServerNames(isExpanding ? [...otherExpandedServerNames, server.name] : otherExpandedServerNames); | ||
| setAreAllExpanded(expandedServerNames.length == serverData.length); |
There was a problem hiding this comment.
Can we use '===' rather than just '==' here?
There was a problem hiding this comment.
Adding onto this, can we also check whether the expandedServerNames length becomes 0? Currently if you expand a row and then collapse it such that there are no expanded rows, the overall "all expanded" carat isn't updating.
nicolethoen
force-pushed
the
table-expandCollapseAll-cleanup
branch
from
4689149 to
deddb1d
Compare
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
The state properly updates when a single folder collapse that results in all folders being collapsed updates the bulk select carat, but seeing a bug where expanding/collapsing a second folder is also toggling the bulk select.
|
@kmcfaul - ah that's interesting. i do notice that it is using the state immediately after setting it. it should use the data that is being used to set the state instead of the state itself... or better yet - an effect. |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Do we want the bulk select carat to show as "expanded" when some of the folders are expanded? Right now it's only showing "expanded" when all of the folders are open.
If so then it looks good to me
|
I think that top carat mean's expand all - so it should be open only when all are expanded? |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
* fix(Table) clean up 'Expand collapse all' demo * update allExpanded on single row toggle * address outstanding comments * refactor collapse/expand all using effect --------- Co-authored-by: nicolethoen <nthoen@redhat.com> Co-authored-by: gitdallas <dallas.nicol@gmail.com>
5 participants
What: Closes #8976
I have also revamped the demo to use functional instead of class component.