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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(notification): introduce high contrast style #2814

Merged

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented May 23, 2019

Fixes #2354.

Changelog

Changed

  • Notification text color to $inverse-01
  • Notification background color to $inverse-02
  • Notification icon color to $inverse-01 (@carbon-design-system/design Does it match your intent?)
  • Notification highlight color to $inverse-support-0x

Testing / Reviewing

Testing should make sure notification looks good.

@netlify
Copy link

netlify bot commented May 23, 2019

Deploy preview for the-carbon-components ready!

Built with commit e352f6c

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

@netlify
Copy link

netlify bot commented May 23, 2019

Deploy preview for carbon-components-react ready!

Built with commit e352f6c

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

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!

v10 enhancements, tech debt, new features automation moved this from In progress to Reviewer approved May 23, 2019
Carbon support automation moved this from PR: needs review 🕵️ to PR: ready to merge 🔜 May 23, 2019
Copy link
Contributor

@shixiedesign shixiedesign 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 getting this done so quickly! @asudoh Just 1 comment,

  • we have agreed to remove drop-shadow on the inline, but keep drop-shadow on toast.
    image

Inline notification got drop-shadow in the previous iteration because of the background color is too close to Gray10 ui-background. Now with high contrast colors, we can safely remove the drop shadow. Thanks!

Comment/question: Can we keep the previous style with colored background as a secondary style? We are seeing potential use cases where users might want the more subtle style, so ideally we can keep both, where High contrast is default/primary. Mentioned this in #2354 (comment) Thanks!

@asudoh asudoh force-pushed the notification-high-contrast-style branch from 70ae352 to 368edcf Compare May 24, 2019 03:57
@asudoh asudoh force-pushed the notification-high-contrast-style branch from 368edcf to 11a6edd Compare May 24, 2019 04:07
@asudoh
Copy link
Contributor Author

asudoh commented May 24, 2019

@shixiedesign Updated the code to remove drop shadow from inline notification. Also implemented "light" variant with old color scheme - Great to have your thoughts on here, esp. would you think of better naming? Thanks!

Copy link
Contributor

@shixiedesign shixiedesign left a comment

Choose a reason for hiding this comment

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

Perfect! 🎉 Thank you so much.

Naming wise,

  • suggestion 1: low-contrast variant. Dark background has been referred to as high-contrast styling.
  • suggestion 2: pastel variant, but on dark themes this wouldn't make sense as much.
  • suggestion 3: say use tinted background?

I proposed these to design team and will return with decision shortly. Let's wait a minute to merge until this is resolved.

@shixiedesign
Copy link
Contributor

Hi @asudoh low-contrast is the name we like! Once naming changes we can merge. Thank you!

@asudoh
Copy link
Contributor Author

asudoh commented May 25, 2019

Thank you for coming up with a name @shixiedesign! Updated.

@asudoh asudoh merged commit 47ca93d into carbon-design-system:master May 25, 2019
v10 enhancements, tech debt, new features automation moved this from Reviewer approved to Done May 25, 2019
Carbon support automation moved this from PR: ready to merge 🔜 to Issue/PR: Closed 🙌 May 25, 2019
@asudoh asudoh deleted the notification-high-contrast-style branch May 25, 2019 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Notification] Add high contrast style
4 participants