-
Notifications
You must be signed in to change notification settings - Fork 377
chore(table): update expandable table data #7838
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
Conversation
|
Preview: https://patternfly-react-pr-7838.surge.sh A11y report: https://patternfly-react-pr-7838-a11y.surge.sh |
nicolethoen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too long ago, the table and table composable examples were refactored so that the table data was structured to look more like it was coming from an API and the data was not stored in the example's state. I'm wondering if this is a good thing to also do with this new data set in the demos. If you want to walk through more of what I mean, let me know! Even the most basic table composable example demonstrates what i'm talking about.
|
@nicolethoen this is a great idea - looking at the syntax in the Basic Table Composable demo: This would be a great standard to follow for other demos and I think we definitely should try to take data out of the state and apply this approach where we can. For this expandable demo specifically, it looks like the data would need to remain in the state, since the |
|
@jenny-s51 can you point me to what's changed and what exactly I should be reviewing from a UX perspective? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcarrano The expandable react demo was updated to use the recently updated table data from https://docs.google.com/spreadsheets/d/1-sqIbCpmFfmID0fByWZSOBpn99ocGyT2KoUZ5B_s3zo/edit#gid=0 as part of patternfly/patternfly-design#1157!
The existing core demo: https://pf4.patternfly.org/components/table/html-demos/expandable/
The existing react demo: https://www.patternfly.org/v4/components/table/react-demos/expandcollapse-all/
And the updated react demo: https://patternfly-react-pr-7838.surge.sh/components/table/react-demos/expandcollapse-all/
|
@jenny-s51 it shouldn't be necessary to couple the isExpanded behavior with the data coming from the API. You'd probably want to handle it like the expandable example does where it maintains an array of |
mcarrano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenny-s51 I think this looks good. One of the ideas we talked about was maybe using labels to represent status just to give examples of how to add an element other than text in a table cell. How easy or hard would that be to do? It's sort of a nice to have, but would be good to add if we can. Or we can follow up at a later date. @mmenestr what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd probably want to handle it like the expandable example does where it maintains an array of expandedRepoNames (line 80 in the example code) and uses that information on line 106 when it's constructing the rows object to pass to the Table.
Thanks for suggesting this approach @nicolethoen ! I've updated the code in the expandable demo file to implement the expandable rows per your comment! I've addressed your feedback regarding #7857 (comment) as well and removed lodash from the data file.
One of the ideas we talked about was maybe using labels to represent status just to give examples of how to add an element other than text in a table cell. How easy or hard would that be to do? It's sort of a nice to have, but would be good to add if we can. Or we can follow up at a later date. @mmenestr what are your thoughts?
This is a great idea @mcarrano - I've updated the demo to represent the status cells using labels, following the mock you linked. @mcarrano @mmenestr please share your thoughts when you get a chance 🙂
|
Still WIP - marking as draft until I fix the padding on the expandable content and a few other remaining visual things |
mcarrano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for making those changes @jenny-s51 !
5586a5f to
a46d2dc
Compare
565b698 to
9e9d582
Compare
tlabaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #7685 , towards #7384, follows investigation done in #7795
The expandable table data must remain hardcoded in the md file because the consumer needs to specify the data that is in the expandable section.
Because the data is hardcoded here, we cannot paginate this without puffing up the demo with data and pagination logic, therefore pagination cannot be supported in this demo.
Link to expandable demo for convenience: https://patternfly-react-pr-7838.surge.sh/components/table/react-demos/expandcollapse-all/