-
Notifications
You must be signed in to change notification settings - Fork 376
chore(dropdown): add split button action example #11753
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(dropdown): add split button action example #11753
Conversation
|
@kmcfaul @thatblindgeye I have created a draft for the example. Let me know of the changes required. |
|
Preview: https://patternfly-react-pr-11753.surge.sh A11y report: https://patternfly-react-pr-11753-a11y.surge.sh Preview: https://pf-react-pr-11753.surge.sh A11y report: https://pf-react-pr-11753-a11y.surge.sh |
|
There were some a11y issues regarding |
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.
Event handling is not quite ideal in this example.
In other dropdown examples, if I open the menu using the keyboard, i can hit tab and it will close the menu and put focus on the next focusable element on the page (in the docs, that's usually the button)
In the other dropdown examples, I can also click outside the menu and it will close the menu.
I think it's because this new example might be missing the 'onOpenChange' prop.
6073673 to
6adca58
Compare
|
Yes, it was happening due to the missing |
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.
I'm noticing that if I select an option from the menu (with they keyboard or my mouse) browser focus is not returned to the MenuToggle as it does in other examples.
I think you're also missing the shouldFocusToggleOnSelect prop
|
When adding the |
|
Looks like it's because we're applying the patternfly-react/packages/react-core/src/components/MenuToggle/MenuToggle.tsx Lines 200 to 202 in 31a4c28
Since it's a plain div without any tabindex focus gets lost when trying to focus it, and we end up back at the top of the page when you Tab again. This PR is what added the ref to that div #8024 . @kmcfaul do you recall if we need to have the ref on this wrapper div? Moving it to the An alternative would be possibly having to supply some custom focus logic in the example code, which consumers would then have to also implement. |
|
Yeah I think this may have been added to the wrong place, it seems like having it on the button would make more sense. I'm a little concerned whether this would count as a breaking change if people are referencing the split actions through the ref, but I think this is more of a bug fix. If we didn't want to change the ref location, we maybe could have the component change how it handles focus if |
|
After discussing with the team, we should keep the |
6adca58 to
4834e6d
Compare
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
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.
From what I recall this is looking good, just couple things below
|
|
||
| ### Split button | ||
|
|
||
| Description of split button |
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.
@edonehoo would you mind taking a stab at quick verbiage here? Would be worth mentioning that the focus must be handled manually when shifting focus back to the toggle element after selecting an item (if the dropdown menu closes upon selection)
| if (toggleRef.current) { | ||
| const toggleButton = toggleRef.current.querySelector('button[aria-expanded]'); | ||
| if (toggleButton) { | ||
| (toggleButton as HTMLElement).focus(); | ||
| } | ||
| } |
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.
Nit: we could remove a layer of nesting here by early returning is !toggleRef.current, or having toggleButton = toggleRef?.current?...
c4000d4 to
2d737dc
Compare
|
|
||
| ``` | ||
|
|
||
| ### Split button |
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.
| ### Split button | |
| ### Split toggle with checkbox |
to align with the language used in the menu toggle docs
|
|
||
| ### Split button | ||
|
|
||
| Description of split button |
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.
| Description of split button | |
| To combine a checkbox or other control with a dropdown menu, use a split button. | |
| A `<MenuToggle>` can be rendered as a split button via `splitButtonItems`. Elements to be displayed before the dropdown toggle button (like the `<MenuToggleCheckbox>`) must be included in the `splitButtonItems`. | |
| If the dropdown menu closes upon selection, you will need to manually shift focus back to the toggle element after a user selects an item from the menu. | |
wdyt about this?
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.
🚢 it
@Mash707 if this looks good to you can you make this quick update?
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.
Done 🚀🚀.
WalkthroughAdds a new split-button Dropdown example and corresponding docs: a React component Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
| ### Split toggle with checkbox | ||
|
|
||
| To combine a checkbox or other control with a dropdown menu, use a split button. | ||
|
|
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.
| A `<MenuToggle>` can be rendered as a split button via `splitButtonItems`. Elements to be displayed before the dropdown toggle button (like the `<MenuToggleCheckbox>`) must be included in the `splitButtonItems`. | |
| If the dropdown menu closes upon selection, you will need to manually shift focus back to the toggle element after a user selects an item from the menu. | |
can you add these 2 lines too?
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.
My bad. I totally missed it in the previous comment. Have added it now 🚀.
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-core/src/components/Dropdown/examples/Dropdown.md(1 hunks)packages/react-core/src/components/Dropdown/examples/DropdownWithSplit.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, test & deploy
- GitHub Check: Build
🔇 Additional comments (3)
packages/react-core/src/components/Dropdown/examples/Dropdown.md (1)
67-77: LGTM! Documentation clearly explains the pattern.The documentation appropriately describes the split button pattern and the manual focus restoration requirement. The verbiage aligns with the team's consensus from the PR discussion.
packages/react-core/src/components/Dropdown/examples/DropdownWithSplit.tsx (2)
16-28: Focus restoration pattern correctly implements the team's consensus approach.The
onFocusfunction usesquerySelectorto find and focus the toggle button, which aligns with the team's decision documented in the PR objectives. This addresses the known issue whereshouldFocusToggleOnSelectdoesn't work withsplitButtonItems.The early return on line 17-19 addresses the previous review feedback to avoid nesting.
34-96: Well-structured example demonstrating split button pattern.The component correctly implements:
- Dual ref handling (lines 40-48) to support both the Dropdown's callback ref and the local
useRefneeded for focus restoration- Required accessibility labels on
MenuToggle(line 52) andMenuToggleCheckbox(line 50) that satisfied the a11y tests per the PR comments- The
onOpenChangehandler (line 37) to sync external state changes, which was identified as missing in early PR feedback- A variety of dropdown item states (action, link, disabled, aria-disabled with tooltip, separated items) that provide a comprehensive example
packages/react-core/src/components/Dropdown/examples/DropdownWithSplit.tsx
Outdated
Show resolved
Hide resolved
edonehoo
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!
Closes #10197
References:
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.