-
Notifications
You must be signed in to change notification settings - Fork 377
chore(Tile): Convert examples to typescript #8043
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
chore(Tile): Convert examples to typescript #8043
Conversation
|
Preview: https://patternfly-react-pr-8043.surge.sh A11y report: https://patternfly-react-pr-8043-a11y.surge.sh |
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.
Hi @XiangyuShen , thank you for your contribution! The tile conversions look awesome! 🎉
I am noticing we have a couple of existing bugs in our Tile docs. They are small, easy to fix, and would be great to squash in this PR before we merge it! Would you be willing to help us make those changes?
- On line 14 of
Tile.md, we need to change
Keyboard interaction patterns and a11y is implemented in the Tile demos, located in the [Demo section](/documentation/react/demos/tiledemo).
to
Keyboard interaction patterns and a11y is implemented in the Tile demos, located in the [Demo section](/components/tile/react-demos).
This will fix the current 404 error with the URL.
- I also noticed an existing bug in our demos... link to the TileDemo.md file.
TheisSelectedfunctionality isn't working properly because the string passed in does not match the existingidprop that is set onTile.
To fix the select logic, we need to make sure they match. E.g. on line 30 of TileDemo.md, change the value for isSelected to be:
isSelected={selectedId === 'single-select-tile-1'}
...and do that for the other three tiles in the demo!
In the Multiple Select demo, we should make sure the .includes param also matches the id.
E.g.:
isSelected={selectedIds.includes('multiselect-tile-1')}
... and so forth for the remaining 3 tiles!
Just those two quick updates - otherwise LGTM!
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.
🚀
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.
LGTM! 🥳
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #8039
Convert Tile examples to TypeScript
Additional issues: