Add overflow menu (ActionMenu) to focus zone in ActionBar
#2259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: cec8edb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
github-actions
bot
added
the
javascript
TylerJDev
commented
Sep 28, 2023
| @@ -111,7 +137,7 @@ export default class FocusGroupElement extends HTMLElement { | |||
| let el: HTMLElement | null = focusEl | |||
| do { | |||
| el = el.closest(`[popover]:not(:popover-open)`) | |||
| if (el?.popover === 'auto') { | |||
| if (el?.popover === 'auto' && !['ArrowRight', 'ArrowLeft'].includes(event.key)) { | |||
There was a problem hiding this comment.
This prevents the left and right arrow keys from triggering a menu. Without, if you were to navigate the ActionBar with arrow keys, the menu would automatically open when either key is pressed.
camertron
previously requested changes
Sep 28, 2023
…primer/view_components into tylerjdev/action-bar-focus-trap
jonrohan
approved these changes
Oct 6, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
PR adds the overflow menu (
ActionMenu) to the focus trap withinActionBar. This is based off of the following issue: https://github.com/github/primer/issues/1131.Screenshots
Before PR, shows overflow menu being reachable only through tab
Screen.Recording.2023-09-28.at.2.13.38.PM.mov
Video shows arrow key navigation through the
ActionBarcomponent. The navigation is trapped within the menu items and does not reach the overflow menu. Video then shows usage of theTabkey, focusing directly onto the overflow menu.Current PR, shows overflow menu being placed within focus trap
Screen.Recording.2023-09-28.at.1.48.29.PM.mov
Video shows arrow key navigation through the
ActionBarcomponent. The navigation is trapped within the component, and reaches the overflow menu without having to pressTabto focus onto it.Integration
List the issues that this change affects.
https://github.com/github/primer/issues/1131
Risk Assessment
What approach did you choose and why?
PR adds
ActionMenuto focus trap withinActionBar. Currently, the menu must be tabbed into separately rather than a part of the focus trap.This also adds the ability in
focus_groupto havetabindexset back onto the invoker button of the popover menu. This fixes a bug inActionBar, where if clicked outside of that menu whilst active, focus would be stuck on amenuitemand not the invoker.Anything you want to highlight for special attention from reviewers?
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.