-
Notifications
You must be signed in to change notification settings - Fork 79
fix(time-picker): high contrast visibility of outlines in focus and hover states #6129
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(time-picker): high contrast visibility of outlines in focus and hover states #6129
Conversation
…trast #6118 * Adds a z-index to the buttons and inputs in hover or focus state * Does not have any noticable change in regular contrast mode
@benelan & @geospatialem I'm not sure how I messed up the semantic PR but I know I did not do anything to the E2E tests that are preventing the checks from passing. |
@mpriour I'm not sure if this'll fix it, but might need to add the component to the fix. Something like:
Will look into the e2e test failure, might just need to re-run them. 🏃🏻 But agreed, does not apply to this fix. Update: Re-ran the e2e tests and the PR is good to go now. Going to take a peek at the fix now, thanks for taking a look @mpriour! cc #3887 |
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.
@geospatialem It looks like I can remove the outline offset and shrink the focus ring in high-contrast mode to just be the size of the input. |
@geospatialem Looks like all the pre-merge checks have passed. Take another look when you get a chance and verify that the focus ring doesn't overlap other parts of the UI in high contrast mode. |
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 in high contrast mode, and in normal viewing mode! 💥
Just need a dev sign off on the updated code style changes.
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.
LGTM! 👍
We have some unstable visual regression tests that were blocking merging. I just cleared those up, so its ready to install. Thanks for the contribution! |
Related Issue: #6118
Summary