Skip to content

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

Merged
merged 6 commits into from
Dec 20, 2022

Conversation

mpriour
Copy link
Member

@mpriour mpriour commented Dec 20, 2022

Related Issue: #6118

Summary

  • ensures that outlines are fully visible in high contrast
  • Adds a z-index to the buttons and inputs in hover or focus state
  • Does not have any noticeable change in regular contrast mode

…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
@mpriour mpriour requested a review from a team as a code owner December 20, 2022 13:34
@mpriour mpriour requested a review from geospatialem December 20, 2022 13:34
@mpriour mpriour changed the title Time-Picker: fix - high contrast visibility of outlines in focus and hover states Time-Picker: fix: high contrast visibility of outlines in focus and hover states Dec 20, 2022
@mpriour mpriour changed the title Time-Picker: fix: high contrast visibility of outlines in focus and hover states fix: Time-picker - high contrast visibility of outlines in focus and hover states Dec 20, 2022
@mpriour
Copy link
Member Author

mpriour commented Dec 20, 2022

@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.

@geospatialem
Copy link
Member

geospatialem commented Dec 20, 2022

I'm not sure how I messed up the semantic PR

@mpriour I'm not sure if this'll fix it, but might need to add the component to the fix. Something like:

fix(time-picker): high contrast visibility of outlines in focus and hover states

did not do anything to the E2E tests that are preventing the checks from passing.

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

Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

Looking great! ✨

Noticed when using input-time-picker the first and last items outline extends a bit outside the component. Very minor, and the time-picker looks good by itself, but wanted to mention if its something we should address in this PR.

input-time-picker-high-contrast-audit

@mpriour
Copy link
Member Author

mpriour commented Dec 20, 2022

@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.

Screen Shot 2022-12-20 at 10 40 14 AM

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 20, 2022
@mpriour mpriour changed the title fix: Time-picker - high contrast visibility of outlines in focus and hover states fix(time-picker): high contrast visibility of outlines in focus and hover states Dec 20, 2022
@mpriour
Copy link
Member Author

mpriour commented Dec 20, 2022

@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.

Copy link
Member

@geospatialem geospatialem left a 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.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@mpriour
Copy link
Member Author

mpriour commented Dec 20, 2022

@benelan @driskull - All checks have passed and required code reviewers have approved code. Why can I not squash merge this PR at this time?

@mpriour mpriour added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 20, 2022
@benelan
Copy link
Member

benelan commented Dec 20, 2022

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!

@mpriour mpriour merged commit 90ddff1 into master Dec 20, 2022
@mpriour mpriour deleted the mpriour/6118-time-picker-high-contrast-visiblity branch December 20, 2022 20:58
@github-actions github-actions bot added this to the 2023 January Priorities milestone Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants