-
Notifications
You must be signed in to change notification settings - Fork 375
chore(table): move legacy examples to deprecated tab #8741
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-8741.surge.sh A11y report: https://patternfly-react-pr-8741-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.
Looks great to me! just one note!
| The second is the original `Table` component. It is configuration based and takes a less declarative and more implicit approach about laying out the table structure, such as the rows and cells within it. | ||
|
|
||
| **For most common use cases, we recommend using `TableComposable`. Both implementations are supported and fully maintained.** | ||
| The legacy `Table` component is configuration-based and takes a less declarative and more implicit approach to laying out the table structure, such as the rows and cells within it. |
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.
use deprecated rather than legacy in this sentence
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.
Looks good! Merge conflict just needs to be resolved and a question below based on Nicole's comment
| The second is the original `Table` component. It is configuration based and takes a less declarative and more implicit approach about laying out the table structure, such as the rows and cells within it. The documentation for the older table implementation can be found under the [React legacy](/components/table/react-legacy) tab. | ||
|
|
||
| **For most common use cases, we recommend using `TableComposable`. Both implementations are supported and fully maintained.** | ||
| The documentation for the deprecated legacy table implementation can be found under the [React deprecated](/components/table/react-deprecated) tab. It is configuration based and takes a less declarative and more implicit approach to laying out the table structure, such as the rows and cells within it. |
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.
Would it also make sense to remove "legacy" from this sentence?
| The documentation for the deprecated legacy table implementation can be found under the [React deprecated](/components/table/react-deprecated) tab. It is configuration based and takes a less declarative and more implicit approach to laying out the table structure, such as the rows and cells within it. | |
| The documentation for the deprecated table implementation can be found under the [React deprecated](/components/table/react-deprecated) tab. It is configuration based and takes a less declarative and more implicit approach to laying out the table structure, such as the rows and cells within it. |
Possibly a similar question for the "react deprecated" tab's heading of "Legacy Table" as to whether that should be updated to "Deprecated Table".
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.
Removed "legacy"!
As far as the heading, ultimately it looks like it makes the most sense to just remove it from both tabs.
When TableComposable is renamed to Table, it won't make much sense to have another "Table" heading nested within the higher-level "Table" heading at the top of the page.
Also since we are communicating in multiple places (the tab, the description, etc.) that the legacy variant is deprecated, we shouldn't need to specify "Deprecated table" again in the heading. Thank you for calling this out - nice catch 🙂 WDYT @thatblindgeye ?
deec918 to
18bf90d
Compare
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.
@jenny-s51 in regards to your reply above, that definitely makes sense! Thanks for making the update, only thing is the pf version mismatch needing to be updated again, otherwise looks good!
delete examples from legacy dir add todo bump PR feedback, remove title
18bf90d to
b28a3ba
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.
Looks great Jenny!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #8742. Towards #8005
Moves legacy
Tableexamples intodeprecatedtab in docs