-
Notifications
You must be signed in to change notification settings - Fork 30
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
refactor(components): menu item only changes color on focus-visible
#2327
Conversation
Deploying atlantis with Cloudflare Pages
|
text-align: start; | ||
background-color: transparent; | ||
cursor: pointer; | ||
gap: var(--menu-space); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This re-order happened via auto-fix
@@ -65,24 +65,26 @@ | |||
|
|||
.action { | |||
display: flex; | |||
gap: var(--menu-space); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width: 100%; | ||
padding: var(--menu-space); | ||
border: none; | ||
border-radius: var(--radius-base); | ||
outline: transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beacuse we're using box-shadow on focus, this sets outline to transparent so that high-contrast modes still work.
} | ||
|
||
.action:hover, | ||
.action:focus { | ||
.action:focus-visible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
focus-visible respects browser heuristics for what should and not be focused
This reverts commit 16c2527.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving, since my suggestion is non-blocking. But I think it would be nice to add transition for box-shadow as well to have it "synced" with background-color.
Co-authored-by: Nazarii L <nazarii.l@getjobber.com>
@nad182 added it, great suggestion! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Motivations
Bring the menu items more in line with our overall pattern for focus states.
Changes
Changed
shadow-focus
instead of outline on focus-visibleScreen.Recording.2025-01-21.at.3.20.12.PM.mov
Testing
Open a menu with your mouse vs with your keyboard
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.