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

Unify focus and list items rendering in dropdown menus #16642

Open
wants to merge 5 commits into
base: ck/16311
Choose a base branch
from

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Jul 1, 2024

Suggested merge commit message (convention)

Internal (ui): Unify dropdown menus focus behavior with menu bar and other menus.
Internal (ui): Use ListItemButtonView instead of ButtonView class as base for dropdown menu buttons.
Internal (ui): Add support for hasIconSpace attribute of ListItemButtonView and allocate space for check marks if there is at least one toggleable menu item in menu.


Additional information

  1. I marked this commit as internal because dropdown menu is not released yet.
  2. Focus behavior has been changed in order to make it unified with menu bar. After this change, hovering menu items moves focus from search input to menu items, and any keyboard interaction with the menu makes it show the focus border.
  3. Why is it based on ck/16311 branch?
    3.1. We don't have manual tests for dropdown menu, and the only way to test that is AI.
    3.2. The presence of search input is important to show how "hover focus" behaves with that kind of dropdown.

Part of PRs:

  1. Add search support to dropdown menus #16314
  2. Create dropdown menu component #16631

Screens

⚠️ Look at the focus placement.

Before:

obraz

After:

obraz

dropdown-focus-keyboard-unify-2024-07-01_13.59.37.mp4

@Mati365
Copy link
Member Author

Mati365 commented Jul 3, 2024

@scofalik This one can be merged directly to core dropdown function. I rebased it on search branch just for tests. There should be no conflicts.

@oleq oleq removed their request for review July 25, 2024 08:10
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.

1 participant