-
Notifications
You must be signed in to change notification settings - Fork 377
Begin deprecate table #8892
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
Begin deprecate table #8892
Conversation
|
Preview: https://patternfly-react-pr-8892.surge.sh A11y report: https://patternfly-react-pr-8892-a11y.surge.sh |
jenny-s51
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.
This is looking awesome @nicolethoen! 🤩 🎉
I think is preferable to rename now while we are in the major release rather that accumulating tech debt. |
thatblindgeye
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.
I'm noticing the following Table demos seem to not be working as expected anymore:
- Bulk Select (the actual table body doesn't have any content rendered inside of it)
- Expand/collpase all (
TypeError: Cannot read properties of null (reading 'length')when opening the demo, plus the same error forreading 'header'in console)
Those could be fixed by the followup that updates usage of deprecated table, though... Other than that this is looking good, though
@nicole which issue will cover this? |
|
cb73e2d to
8a1f290
Compare
|
@thatblindgeye I opened a follow up issue to fix those demos since - when looking at them, it's not super obvious what's wrong and it'd be better to get this PR in quick as we can to simplify merge conflicts in the future. |
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.
Thank you Nicole and Jenny! Lgtm
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
PF/TableComposable has been renamed to PF/Table. Related to * patternfly/patternfly-react#8005 * patternfly/patternfly-react#8892 Based on the changes proposed by https://github.com/patternfly/pf-codemods
PF/TableComposable has been renamed to PF/Table. Related to * patternfly/patternfly-react#8005 * patternfly/patternfly-react#8892 Based on the changes proposed by https://github.com/patternfly/pf-codemods
PF/TableComposable has been renamed to PF/Table. Related to * patternfly/patternfly-react#8005 * patternfly/patternfly-react#8892 Based on the changes proposed by https://github.com/patternfly/pf-codemods
What: Closes #8005
in order to build this PR, I had to temporarily remove the building of Props tables and CSS variables tables on the component docs (they were built using the now deprecated table). Once this merges, I can reimplement them using the recommended Table and they will return.
I'm curious if people are averse to the idea of reverting the name change of the TableComposable. If we keep it's old name, this PR will be smaller, it will be easier to write codemods, and it will be easier to process which table a consumer is trying to use. I might propose keeping the name TableComposable until the day comes when we are ready to remove the deprecated Table from the code base entirely. @tlabaj WDYT?