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.

Sign up for GitHub

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

Jump to bottom

fix(notification): update focus styles for low contrast #5978

Merged
merged 9 commits into from
May 12, 2020
Merged

fix(notification): update focus styles for low contrast #5978

merged 9 commits into from
May 12, 2020

Conversation

joshblack
Copy link
Contributor

Closes #5977

Updates focus styles for low-contrast to be applied on :focus

Changelog

New

Changed

  • Update toast and inline low contrast styles to include focus

Removed

@joshblack joshblack requested a review from a team as a code owner April 29, 2020 16:38
@ghost ghost requested review from asudoh and emyarod April 29, 2020 16:38
@netlify
Copy link

netlify bot commented Apr 29, 2020

Deploy preview for carbon-elements ready!

Built with commit 5193768

https://deploy-preview-5978--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 29, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 2e8a7d5

https://deploy-preview-5978--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 29, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 2e8a7d5

https://deploy-preview-5978--carbon-components-react.netlify.app

tw15egan
tw15egan approved these changes Apr 29, 2020
View reviewed changes
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that 👍 ✅

emyarod
emyarod approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me, should this get a visual review as well?

@tw15egan tw15egan requested review from a team and laurenmrice and removed request for a team April 30, 2020 19:24
laurenmrice
laurenmrice requested changes Apr 30, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

There is a slight haze/overlay when focusing on theaction button in the notification. There should also be a 2px border around the action button, it looks like its at 1px.

Apr-30-2020 15-04-14

@joshblack
Copy link
Contributor Author

@laurenmrice I believe the haze is coming from the styles for the ghost button if I understand correctly. Would we want to remove them?

For the outline, it should be using the 2px focus styles 👀

image

@laurenmrice
Copy link
Member

When I view the ghost button story and apply focus, it is not applying that visual overlay/haze look. I think this has something to do with how we are treating it in the notification component.

When I was referring to 2px focus outline, the close icon outline is correct, it is the Action button that needs the 2px outline, it is currently at 1px. This is also different from how the ghost button story is, so I think it is only happening here.

@tw15egan tw15egan linked an issue May 11, 2020 that may be closed by this pull request
[Notification] Focus outline now permanent on low contrast notifications #6053
Closed
@joshblack joshblack mentioned this pull request May 12, 2020
Closed
@joshblack
Copy link
Contributor Author

@laurenmrice Sounds good! Thanks so much for clarifying. Let me take a look 😄

laurenmrice
laurenmrice approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Thank you Josh ! Everything looks great 🙌🏻

@joshblack joshblack merged commit 3b94f3c into master May 12, 2020
@joshblack joshblack deleted the joshblack-patch-1 branch May 12, 2020 20:44
joshblack added a commit that referenced this pull request May 13, 2020
@joshblack @tw15egan
fix(notification): update focus styles for low contrast (#5978)
41267c8 
* fix(notification): update focus styles for low contrast

* Update _inline-notification.scss

* fix(notification): update focus styles for action button

* fix(notification): update hover and focus styles for action button

* fix(notification): update colors according to new spec

Co-authored-by: TJ Egan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@tw15egan tw15egan tw15egan approved these changes

@emyarod emyarod emyarod approved these changes

@asudoh asudoh Awaiting requested review from asudoh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@joshblack @laurenmrice @emyarod @tw15egan