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

fix(menu-item): tweak active and focused styles #1639

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Nov 25, 2024

This PR contains two loosely related changes:

Remove active style from menu item with tabIndex 0

Setting the active class on items with tab index 0 works nicely when using keyboard navigation but causes odd behaviour when using mouse-based interaction: a Menu with MenuItems can end up having multiple "active items":
Screenshot 2024-11-25 at 15 39 41
By removing this code the keyboard navigation is still visually supported by the focus outline, which is enough IMO.

Fix focussed outline styles

The problem was as follows:

  • The menu-item styles included focus styles for the <a> tag
  • This makes sense because these are "naturally focusable elements", similar to a button.
  • However now that we have keyboard navigation, the parent <li> gets a tab-index and the <li> becomes the element that receives focus

So I've moved the focus styles from the <a> to the <li> and everything looks better.

Screenshot 2024-11-26 at 09 45 57

For reference, here's a screenshot of how the focussed menu-item looked before, with the broken outline styles and the grey background:
Screenshot 2024-11-25 at 17 40 28

@HendrikThePendric HendrikThePendric requested a review from a team as a code owner November 25, 2024 16:35
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Nov 25, 2024

🚀 Deployed on https://pr-1639--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify November 25, 2024 16:39 Inactive
@HendrikThePendric HendrikThePendric force-pushed the fix/remove-active-style-from-menu-item-with-tabindex-0 branch from b6ce9b9 to 8a52497 Compare November 25, 2024 16:54
@dhis2-bot dhis2-bot temporarily deployed to netlify November 25, 2024 16:58 Inactive
@HendrikThePendric HendrikThePendric force-pushed the fix/remove-active-style-from-menu-item-with-tabindex-0 branch from 8a52497 to a91c77b Compare November 25, 2024 17:03
Copy link

sonarcloud bot commented Nov 25, 2024

@dhis2-bot dhis2-bot temporarily deployed to netlify November 25, 2024 17:07 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 26, 2024 08:41 Inactive
@HendrikThePendric HendrikThePendric changed the title fix: remove active style from menu item with tabIndex 0 fix(menu-item): tweak active and focussed styles Nov 26, 2024
@HendrikThePendric HendrikThePendric changed the title fix(menu-item): tweak active and focussed styles fix(menu-item): tweak active and focused styles Nov 26, 2024
@HendrikThePendric HendrikThePendric merged commit e6bf884 into master Nov 26, 2024
24 checks passed
@HendrikThePendric HendrikThePendric deleted the fix/remove-active-style-from-menu-item-with-tabindex-0 branch November 26, 2024 09:22
dhis2-bot added a commit that referenced this pull request Nov 26, 2024
## [10.1.1](v10.1.0...v10.1.1) (2024-11-26)

### Bug Fixes

* **menu-item:** tweak active and focused styles ([#1639](#1639)) ([e6bf884](e6bf884))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 10.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants