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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(notifications): fix pointer events for inline low contrast, update inverse link colors #4805

Merged
merged 15 commits into from
Dec 10, 2019
Merged

fix(notifications): fix pointer events for inline low contrast, update inverse link colors #4805

merged 15 commits into from
Dec 10, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Dec 2, 2019

Closes #4804

This PR fixes the pointer events issue identified in #4804 and seen on the gatsby-theme-carbon site by adding pointer-events: none to the pseudo-selector :before styles causing the problem.

I also updated the storybook to add example links to both Notification components' subtitle prop, so that links can easily be checked in the future.

Also in the process of doing this, I noticed that links passed into title or subtitle props aren't styled correctly with the $inverse-link token, so I went ahead and added styles for that. I'm just not sure what hover/focus color you want for those links? Just let me know & I can update it 馃憤

Changelog

New

  • use $inverse-link color tokens for links inside InlineNotification and ToastNotification

Changed

  • for low contrast InlineNotification, add pointer-events:none to the pseudo :before that covers the component
  • add link example to both notification components in storybook

Testing / Reviewing

Make sure to select the Low Contrast knob in storybook. I definitely suggest comparing the deploy with InlineNotification usage on the gatsby-theme-carbon site 馃憤

Also after you toggle on Low Contrast knob in this PR's storybook deploy, you can inspect the low-contrast :before psuedo selector & comment out the pointer-events: none style, then see if you can click on the link inside the InlineNotification (you shouldn't be able to)

@jendowns jendowns requested a review from a team as a code owner December 2, 2019 18:48
@ghost ghost requested review from aledavila and asudoh December 2, 2019 18:48
@netlify
Copy link

netlify bot commented Dec 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit 009ae2e

https://deploy-preview-4805--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Dec 2, 2019

Deploy preview for carbon-elements ready!

Built with commit 009ae2e

https://deploy-preview-4805--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Dec 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit 75f3b92

https://deploy-preview-4805--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Dec 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit 009ae2e

https://deploy-preview-4805--carbon-components-react.netlify.com

@asudoh asudoh requested a review from a team December 2, 2019 22:02
@ghost ghost requested review from jillianhowarth and removed request for a team December 2, 2019 22:02
@joshblack
Copy link
Contributor

One thing that @dakahn pointed out during our PR review was that the links seemingly don't hit color contrast, did you notice this too @jendowns?

image

@jendowns
Copy link
Contributor Author

jendowns commented Dec 5, 2019

Thanks for your notes on the colors @abbeyhrt & @joshblack! I updated the inverse-link color rules I added to only apply to the non-low-contrast version of the components.

I also went ahead and added some hover and focus styles for links inside notifications... I realize it would be ideal if people passed in the Link component in this situations, but I think it's also super common for someone to just pass in an plain anchor. Let me know what you think!

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thank you Jen!!

Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Ok I think this looks good. No color change with just the added underline on hover is good.

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

Successfully merging this pull request may close these issues.

[Notification]: low-contrast styles block inline notification's pointer events
6 participants