-
Notifications
You must be signed in to change notification settings - Fork 30
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
Make SubNav SubMenus accessible using keyboard #679
Conversation
🦋 Changeset detectedLatest commit: f1264f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
|
60eb773
to
7f7faee
Compare
7f7faee
to
d672a88
Compare
d672a88
to
92f36be
Compare
92f36be
to
c539e70
Compare
3b6a104
to
b902613
Compare
b902613
to
d623424
Compare
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 great 🙌
6a50d02
to
f1264f9
Compare
Summary
Makes the submenu's arrow icon focusable, activating it (with Space or Enter) will open the submenu. Tabbing to the next or previous item will collapse the submenu. Esc intentionally doesn't close the menu to match the behaviour of the WAI example referenced for this change.
List of notable changes:
useIsChildFocused
hook to detect whether a given element has a child which is focusedSubNavLinkWithSubmenu
fromSubNavLink
component.What should reviewers focus on?
onKeyDown
event to detect when Space or Enter are pressed, rather than usingonClick
. The reason is that I don't want the caret to be clickable, as that would differ from the component's existing behaviour. UsingonClick
would allow users to click the caret and keep the menu open until they click away. If this is the desired behaviour, I'd be happy to switch toonClick
instead, however my concern is that the arrow icon would be quite a small tap target.Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
Reviewer checklist:
Screenshots:
Export-1722930600800.mp4
Export-1722930428822.mp4