chore(DataList): convert examples to TypeScript #7154
chore(DataList): convert examples to TypeScript #7154nicolethoen merged 7 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-7154.surge.sh A11y report: https://patternfly-react-pr-7154-a11y.surge.sh |
jenny-s51
changed the title
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Only a few comments below, a couple of requested changes and one nitpick. Also the comment I left on the DatePicker conversion PR applies here, so depending on your response to that (regarding the export names from each example) that may apply here, too.
| if (allExpanded) { | ||
| setExpanded(['ex-toggle1', 'ex-toggle2', 'ex-toggle3']); | ||
| } else { | ||
| setExpanded([]); | ||
| } |
There was a problem hiding this comment.
This if...else block would work better in a useEffect I believe. Currently the arrow icon for the expand/collapse all toggle doesn't really match the actual allExpanded state; if all items are expanded the expand/collapse all toggle has a right arrow when it should have a down arrow, for example.
There was a problem hiding this comment.
Nice catch! Great idea.
| return ( | ||
| <> | ||
| <div key="example-1"> | ||
| <h2>Default fitting - example 1</h2> |
There was a problem hiding this comment.
I think it'd be worth updating this (and the other sub-examples in this section) to an <h4> element
There was a problem hiding this comment.
👍 Updated! Definitely looks better - thanks!
| Array.from({ length: count }, (v, k) => k).map(k => ({ | ||
| id: `item-${k}`, | ||
| content: `item ${k} `.repeat(k === 4 ? 20 : 1) |
There was a problem hiding this comment.
Nitpick: may be something worth fixing in another issue, but I think it'd be worth giving these v and k params more meaningful names
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Updates look great!
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
Looks great!
Could you rebase your PR? I think there is a new Data List example that we should be sure we don't leave out of the typescript conversions :)
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
You are just missing one file in your MD file. Looks good otherwise.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
What: Closes #7139
Additional issues: