patternfly-react
patternfly-react copied to clipboard
chore(dropdown): add split button action example
Closes #10197 References:
- https://v5-archive.patternfly.org/components/menus/dropdown/react-deprecated#split-button-checkbox
- https://www.patternfly.org/components/menus/menu-toggle#split-toggle-with-checkbox
@kmcfaul @thatblindgeye I have created a draft for the example. Let me know of the changes required. Also could you guys also suggest the description and sub heading for the example, I am not sure of it. Thank You
Preview: https://patternfly-react-pr-11753.surge.sh
A11y report: https://patternfly-react-pr-11753-a11y.surge.sh
There were some a11y issues regarding MenuToggle and MenuToggleCheckbox since I didn't add aria-label to them.
@thatblindgeye do check the aria-labels once, I have added them for now so that the tests don't fail.
Yes, it was happening due to the missing onOpenChange prop. I have added it and now the menu is closing when clicking outside.
When adding the shouldFocusToggleOnSelect , it does not seem to work with splitButtonItems prop of the menu toggle. If I remove the splitButtonItems prop then it is working fine.
I also added splitButtonItems to others examples to test things out and it breaks them too.
Looks like it's because we're applying the innerref inside MenuToggle to that wrapper div instead of the toggle button:
https://github.com/patternfly/patternfly-react/blob/31a4c28eaa752338c9f9c1ca8cebbaa005037e72/packages/react-core/src/components/MenuToggle/MenuToggle.tsx#L200-L202
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 https://github.com/patternfly/patternfly-react/pull/8024 . @kmcfaul do you recall if we need to have the ref on this wrapper div? Moving it to the button toggle for the splitButtonItems return seems more accurate in this case, but can't remember if there was a specific reason.
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 splitActionItems is present and send focus back to the toggle button (like via querySelector getting the last button or have another ref applied to the toggle button).
After discussing with the team, we should keep the ref on the div as is, and update Dropdown's logic to handle sending focus back to the toggle button via a querySelector (see the filter demos) for its default behavior.
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.