patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

chore(dropdown): add split button action example

Open Mash707 opened this issue 7 months ago • 8 comments

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

Mash707 avatar Apr 10 '25 18:04 Mash707

@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

Mash707 avatar Apr 10 '25 18:04 Mash707

Preview: https://patternfly-react-pr-11753.surge.sh

A11y report: https://patternfly-react-pr-11753-a11y.surge.sh

patternfly-build avatar Apr 10 '25 18:04 patternfly-build

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.

Mash707 avatar Apr 10 '25 18:04 Mash707

Yes, it was happening due to the missing onOpenChange prop. I have added it and now the menu is closing when clicking outside.

Mash707 avatar Apr 23 '25 19:04 Mash707

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.

Mash707 avatar Apr 28 '25 18:04 Mash707

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.

thatblindgeye avatar Apr 28 '25 19:04 thatblindgeye

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).

kmcfaul avatar May 14 '25 19:05 kmcfaul

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.

kmcfaul avatar May 20 '25 15:05 kmcfaul

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jul 29 '25 11:07 github-actions[bot]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Oct 10 '25 11:10 github-actions[bot]