chore(Menu): convert examples to TypeScript #7627
Conversation
|
Preview: https://patternfly-react-pr-7627.surge.sh A11y report: https://patternfly-react-pr-7627-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Looking good so far! Had some comments for issues I noticed below, as well as the following:
- For most of the examples, the
onSelectmethod doesn't have any types on the parameters like the MenuWithCheckbox file does.
packages/react-core/src/components/Menu/examples/MenuWithFavorites.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Menu/examples/MenuWithDrilldownBreadcrumbs.tsx
Show resolved
Hide resolved
| return validItem; | ||
| }; | ||
|
|
||
| const loadMoreOptions = _stateCallback => { |
There was a problem hiding this comment.
When clicking the "view more" button, focus doesn't get set properl. Currently on the live site, clicking view more will place focus on the next active item that gets rendered in, whereas here that isn't the case.
It looks like it's because this stateCallback param is never actually being called anymore, so whatever callback is passed into loadMoreOptions isn't called.
There was a problem hiding this comment.
Thank you for your review @thatblindgeye!
Addressed most of your comments. I'm investigating the focus issue you mentioned, which doesn't have a trivial solution.
The TSX/React Hooks version of the setState callback is useEffect, but combining useEffect with refs doesn't work the way we'd expect, so I will likely have to rewrite some logic in the ViewMore demo. Currently a WIP!
|
Per discussion with @jenny-s51, I pushed a potential fix to the issue that arose with the View More example after converting to TS/functional components. One area of concern might be if the |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Looks good!
There is a bit of a keyboard issue with drilldown + breadcrumbs I'm noticing where shift tabbing while within a drilled in menu won't immediately take focus to the breadcrumbs (requiring multiple shift tab keypresses). It looks like this was also present in our current docs so it wasn't the conversion, I believe it's probably to do with drilldown replacing the active menu in place so it must shift tab through previous menu containers before hitting the breadcrumbs. Since it's not a new bug I think it's probably outside this PR scope and should be a follow up/separate issue.
jenny-s51
force-pushed
the
menuTS
branch
2 times, most recently
from
4fcaa2c to
22672fd
Compare
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Thank you for adding back your updates @thatblindgeye !
@kmcfaul Ah nice catch - definitely a weird bug! I'm seeing that behavior persist in the existing docs as well. Since you're the one who caught this, would you like to open the related issue?
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Conflicts should now be resolved - did a big rebase to pull in @nicolethoen 's updates from https://github.com/patternfly/patternfly-react/pull/7604/files
|
@jenny-s51 Sure will do. Edit - opened #7756 |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
What: Closes #7570