-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat(action-popover): allow menu button to be focused programmatically #7128
base: master
Are you sure you want to change the base?
Conversation
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.
One minor suggestion (more from a cleanliness perspective) but works great 👍
// Keyboard commands implemented as recommended by WAI-ARIA best practices | ||
// https://www.w3.org/TR/wai-aria-practices/examples/menu-button/menu-button-actions.html | ||
|
||
const onButtonKeyDown = useCallback( | ||
(e) => { | ||
if ( | ||
Events.isSpaceKey(e) || | ||
Events.isDownKey(e) || | ||
Events.isEnterKey(e) | ||
) { | ||
e.preventDefault(); | ||
e.stopPropagation(); | ||
setFocusIndex(firstFocusableItem); | ||
setOpen(true); | ||
} else if (Events.isUpKey(e)) { | ||
e.preventDefault(); | ||
e.stopPropagation(); | ||
setFocusIndex(lastFocusableItem); | ||
setOpen(true); | ||
} | ||
}, | ||
[firstFocusableItem, lastFocusableItem, setOpen], | ||
); |
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.
suggestion (non-blocking): Feels like this can be better written to remove some of the duplicated lines (just for neatness and readability, though - it's functionally fine). Something like this (via Co-Pilot):
const onButtonKeyDown = useCallback(
(e) => {
if (
Events.isSpaceKey(e) ||
Events.isDownKey(e) ||
Events.isEnterKey(e) ||
Events.isUpKey(e)
) {
e.preventDefault();
e.stopPropagation();
setFocusIndex(
Events.isUpKey(e) ? lastFocusableItem : firstFocusableItem
);
setOpen(true);
}
},
[firstFocusableItem, lastFocusableItem, setOpen],
);
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.
Also, a re-read indicates the original code was already there and this "change" was just because it moved, so feel free to completely ignore 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.
Will add this in for some cleanliness
d81070d
Exposes `focusButton` method to allow consumers to programmatically focus on the menu button using ref. fix #5589
a3d8d23
fix #5589
Proposed behaviour
Expose
focusButton
method to programmatically focus on the menu button using ref.Current behaviour
ActionPopover
menu button cannot be focused programmatically.Checklist
d.ts
file added or updated if requiredQA
Additional context
Testing instructions