feat(action-bar): support for action-menus #3780
Merged
Conversation
This file contains hidden or 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
1 task
Contributor
Contributor
Tachometer resultsChromeaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
alert-dialog permalink
avatar permalink
badge permalink
button-group permalink
button permalink
card permalink
checkbox permalink
color-area permalink
color-slider permalink
color-wheel permalink
dialog permalink
field-label permalink
grid permalink
infield-button permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
Firefoxaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
alert-dialog permalink
avatar permalink
badge permalink
button-group permalink
button permalink
card permalink
checkbox permalink
color-area permalink
color-slider permalink
color-wheel permalink
dialog permalink
field-label permalink
grid permalink
infield-button permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
|
TarunAdobe
force-pushed
the
feat/action-menu-in-action-bar
branch
from
87ef5ab to
c6e1604
Compare
najikahalsema
previously requested changes
Nov 8, 2023
Member
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Any clue why these VRTs are changing? That seems unexpected to me. I'm fine with the background changing since I believe that is supposed to match the other action buttons in the action group, but I'm curious about the corners being unrounded. Here's a link to the story in your branch and a link to what's live now.
Also, in order to make sure the addition of the action menu to the ActionBar stories is captured by the VRTs, can you add one extra story that's called "With Action Menu" or something, and have that story start with the ActionMenu in an open state?
Please investigate why the tests are failing. Looks like there's some accessibility issues that need addressing. I think it'd be good to add some tests. Generally, we want to add tests for every new feature we add to a component. A place to start... I did notice that the tab order is broken in your PR in the stories I linked previously, because I can't tab into the action group before the action menu. It also doesn't work as expected in the ActionBar's default story. Some stories regarding this tab index behaviour are probably a good thing to cover in the test file.
Westbrook
reviewed
Nov 11, 2023
Contributor
Westbrook
left a comment
Westbrook
left a comment
There was a problem hiding this comment.
NOTE: for this to be accurately supported in Action Bar, it needs to be accurately supported in Action Groups. Not only does it need to accept the size, static, etc. attribute from the Action Group, but it should play as expected into the keyboard interaction model of the group as well.
1 task
TarunAdobe
force-pushed
the
feat/action-menu-in-action-bar
branch
from
0439ba0 to
301cac6
Compare
TarunAdobe
force-pushed
the
feat/action-menu-in-action-bar
branch
from
1c5b0ea to
92d6693
Compare
TarunAdobe
force-pushed
the
feat/action-menu-in-action-bar
branch
from
92d6693 to
27723e1
Compare
Contributor
|
In https://feat-action-menu-in-action-bar--spectrum-web-components.netlify.app/storybook/?path=/story/action-bar--default when I navigate to the Action Menu with the keyboard, I am unable to access any of the Menu Items once it is opened. |
TarunAdobe
force-pushed
the
feat/action-menu-in-action-bar
branch
from
1756fd2 to
dc2936d
Compare
nikkimk
requested changes
Jun 11, 2024
Contributor
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
Action Menu is not focusable via keyboard within action menu:
- Go to https://feat-action-menu-in-action-bar--spectrum-web-components.netlify.app/storybook/?path=/story/action-bar--fixed
- Navigate to the action bar via keyboard, so that the Close button has focus.
- Press Tab, so that the Edit button has focus.
- Press Tab again, and the More action menu should have focus, but instead focus is moved outside of the action bar to the System Picker after it.
Contributor
Author
|
@nikkimk This is an expected behaviour as we've placed the edit button and the action-menu in the action-group so you should be able to use arrow keys to navigate between the two items inside of the action-group but using tab will jump to the next focusable element which in this case is the picker. |
Contributor
Author
|
Here's the attached documentation for these changes. |
blunteshwar
reviewed
Jun 27, 2024
Rajdeepc
previously approved these changes
Jul 9, 2024
Contributor
Author
|
@Rajdeepc I have addressed all the comments on the PR. Please take a look. |
Rajdeepc
approved these changes
Jul 15, 2024
Contributor
|
LGTM |
10 tasks
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.
Description
Action Menu is not currently an accepted child of Action Bar. Expanding the Action Group management that is internally leveraged by Action Bar to include Action Menu would enable clients to use Action Menu with slot "buttons" and may also help resolve the Menu overlay overlap issue.
Related issue(s)
Motivation and context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.