Skip to content
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 highlight Actions menu items (Edit and Delete) buttons #2332

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

RoyEJohnson
Copy link
Contributor

@RoyEJohnson RoyEJohnson commented Aug 21, 2024

DISCO-307
For clarity, changed ol to menu
Also simplified return value of DropdownItem.

@RoyEJohnson RoyEJohnson requested a review from a team as a code owner August 21, 2024 17:50
@RoyEJohnson RoyEJohnson requested a review from jivey August 21, 2024 17:50
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 21, 2024 17:50 Inactive
@RoyEJohnson
Copy link
Contributor Author

Lint had a problem with the "Stop containers" phase.
image

@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 21, 2024 22:26 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 22, 2024 16:41 Inactive
Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The markup changes are great. I need some clarification regarding the card, shouldn't the first menu item get focused?

There's also a very minor style regression in the MH modal
image

@RoyEJohnson
Copy link
Contributor Author

RoyEJohnson commented Aug 27, 2024

As I read the comment, the focus concern is that the "dialog" (the DisplayNote) doesn't get focus when it appears. That is by design; it's what alt-H is for.
In the Disclosure Pattern, the focus does not automatically move from the control button. The user can open and close it repeatedly. They tab to the first revealed item. I'm not finding that to be a hard and fast rule, but it's what I recall LA communicated to us before: they wanted navigation to proceed to the first item. But it's possible I misunderstood and should make the first item focus upon opening.
I'll figure out the style regression.

@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from f0295ce to d76dda2 Compare August 27, 2024 14:25
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 27, 2024 14:26 Inactive
@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from d76dda2 to 7375997 Compare August 27, 2024 14:41
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 27, 2024 14:41 Inactive
@RoyEJohnson RoyEJohnson requested a review from jivey August 27, 2024 15:07
@RoyEJohnson
Copy link
Contributor Author

I'm going to have it move focus to the first item.

@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 28, 2024 15:41 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 28, 2024 15:55 Inactive
@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from f39e405 to ce568dc Compare August 28, 2024 16:36
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--o7tkxl August 28, 2024 16:36 Inactive
@RoyEJohnson
Copy link
Contributor Author

RoyEJohnson commented Aug 28, 2024

@jivey Focusing on the first item can result in the hover-highlight competing with the focus-highlight, so I had to add code to make focus follow the mouse within the menu. It's ready to review.

Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The styles look good, but with the hover change, it can get into a weird state where one item is highlighted by the mouse and the other the keyboard:
image

I'm not sure how react-aria-component's Menu handles it internally, but that seems like a good interaction goal if we can make it work like that.

@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from 86411dc to 8c24ab4 Compare September 4, 2024 20:07
@RoyEJohnson
Copy link
Contributor Author

I resolved the focus/hover issue by styling focus and not hover.

Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and feels good to use, thanks for going through those review rounds with me.

@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from 32486b2 to e4055ab Compare September 17, 2024 14:24
@Malar-Natarajan Malar-Natarajan temporarily deployed to rex-web-focus-on-first--xvvk8t September 17, 2024 16:14 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--xvvk8t September 17, 2024 21:38 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--xvvk8t September 17, 2024 22:02 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--xvvk8t September 18, 2024 18:25 Inactive
@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from c1d0a5f to 4b0421c Compare September 18, 2024 18:28
@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from e0feeaa to dd06dbe Compare October 2, 2024 14:38
@RoyEJohnson RoyEJohnson force-pushed the focus-on-first-item-in-MH-dialog branch from df0b54f to 5ca988a Compare October 2, 2024 16:07
@TomWoodward TomWoodward temporarily deployed to rex-web-focus-on-first--mteinp November 12, 2024 18:22 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants