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

Update notice colors to pass WCAG AA guidelines #17796

Merged
merged 9 commits into from
Nov 16, 2017

Conversation

jameskoster
Copy link
Contributor

The contrast between white text and the colors used for the backgrounds of our notices do not meet WCAG AA guidelines for contrast ratio. This PR addresses that for success, error and warning notices. The blue notices use a brand color which will need more input before we change it.

Before:

oldnotices

After:

newnotices

Clearly these colors are quite a bit darker and this does change the overall tone somewhat. If we wanted to keep lighter colors the only option really would be to use much lighter backgrounds with darker text colors.

@jameskoster jameskoster added Accessibility (a11y) [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Sep 5, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] S Small sized issue label Sep 5, 2017
@folletto
Copy link
Contributor

folletto commented Sep 7, 2017

Thanks for pushing this forward.

I agree, these colors are unfortunately off our palette, we need some alternative solution... I think trying light with dark text and maybe use the bright colors for borders/side border/else might be worth a try...

@jameskoster
Copy link
Contributor Author

jameskoster commented Sep 7, 2017

Here's an alternative version with light bg / dark text;

dark-text

The contrast with the blue notice which still has white text bugs me, but overall this feels more inline with our palette than the previous darker background approach. Some final tweaks probably required. I would want to change our blue notices to make them consistent before merging this though and that's a bigger discussion.

How do you feel about this direction?

@folletto
Copy link
Contributor

folletto commented Sep 7, 2017

This seems a far better direction! :)

I'd /cc in @mattmiklic as I recall him being involved with excellent color insight in previous discussions around our brand :)

@kellychoffman
Copy link
Member

Here's an alternative version with light bg / dark text;

This looks better than the first direction. I'm not strong on color, so I'll leave it up to the pros.
cc @drw158, although I believe he's still on sabbatical.

@jameskoster
Copy link
Contributor Author

jameskoster commented Sep 7, 2017

Here's a fuller screenshot for context of how the colors sit in the UI;

colors

colors2

global-notices

@folletto
Copy link
Contributor

folletto commented Sep 7, 2017

They look good. :)

...but... these notices should show up at the top, never inside cards... :P

@jameskoster
Copy link
Contributor Author

True, added a couple of other screenshots. I know the positioning isn't quite right but you can at least see the colors ;p

@mattmiklic
Copy link
Member

These look pretty good to me; since I'm colorblind I won't offer any subjective thoughts on the visual appearance, but they are all nicely readable, and no problem distinguishing these shades of red and green.

@rickybanister
Copy link

Since you're modifying the _colors definition itself, can you share other screenshots where the colors are affected—I imagine they are plentiful.

Is it possible to use an scss modifier on the existing colors just for notices to achieve this same effect? This would preserve those colors for use elsewhere (icons, etc) and only modify them for use as a text background. With a lighten() or similar modification they will still sit nicely together (I'm not sure if that's already how you arrived at these colors since this PR just swaps the hexes, so apologies if you were already thinking along those lines).

I also notice that this PR only resets the background color of the notice, we will need to update the notice and globalNotice components' text colors before this can be merged or else you'll have white text on the even lighter backgrounds, no?

@rickybanister
Copy link

Apologies for not fully understanding that your still just exploring the colors—feel free to disregard my feedback about the PR itself!

Looking at the new brand illustrations and their tints of the colors might help us align the notices with the rest of Calypso.

Glad to see us fixing some of the readability.

@jameskoster
Copy link
Contributor Author

jameskoster commented Sep 8, 2017

Since you're modifying the _colors definition itself, can you share other screenshots where the colors are affected—I imagine they are plentiful.

In the latest iteration (not pushed to the branch yet) I've left the variables intact. I'll push that now so you can check out the implementation.

I also notice that this PR only resets the background color of the notice, we will need to update the notice and globalNotice components' text colors before this can be merged or else you'll have white text on the even lighter backgrounds, no?

Light bg notices are un-touched.

light

@matticbot matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Sep 8, 2017
@kellychoffman
Copy link
Member

Looks like the color change only effects the notices, which is nice as there's a bunch of other spots that use the red,green,yellow. Example:

screen shot 2017-09-08 at 11 23 27 am

Example of a success message below. Does it look too minty? cc @kjellr who's a color pro

screen shot 2017-09-08 at 11 05 50 am

We'll also have to update the compact notices. Example:

screen shot 2017-09-08 at 11 15 01 am

@kjellr
Copy link
Contributor

kjellr commented Sep 8, 2017

Example of a success message below. Does it look too minty? cc @kjellr who's a color pro

It does look pretty minty, but I don't think that's a bad thing. We're actually using minty greens in some of the newer illustrations that @cristelrossignol has added recently:

artboard
(from plan-premium.svg and security.svg)

Maybe we should match the greens we use there? #44d0a3, #7bdebf, #d0f4e7

@kellychoffman
Copy link
Member

Ohhh I hadn't seen those. Those look pretty nice.

@jameskoster
Copy link
Contributor Author

Ah, nice! I'll update to use those greens and tweak the other colors accordingly.

@jameskoster
Copy link
Contributor Author

jameskoster commented Sep 20, 2017

OK, I had a play with those colors. The lighter color (#d0f4e7) is simply too light. It doesn't stand out against the background and thus fails as a notice.

The other two colors pretty much share the strength of our original success color. So there's not a lot of benefit switching them.

So as an alternative, how about we keep the backgrounds, but make the text darker? The benefit of this is that it allows us to keep the WordPress blue (info) notice but still update it to pass AA guidelines.

Before:

After:

Ironically, the white text is clearer to my eye, especially on the blue / red, even though it doesn't meet the required contrast ratio. I'm interested to hear what y'all think.

@folletto
Copy link
Contributor

Ironically, the white text is clearer to my eye, especially on the blue / green, even though it doesn't meet the required contrast ratio. I'm interested to hear what y'all think.

I share the same impression.

Either way, they both seem good to me.

@kellychoffman
Copy link
Member

The black text on blue passes? Its pretty hard to read, at least for me.

@apeatling
Copy link
Member

@jameskoster LGTM this one I think?

@aidvu
Copy link
Contributor

aidvu commented Nov 16, 2017

@jameskoster this broke notices for domains:
broken notices

@klimeryk
Copy link
Contributor

Is it just me or did this change actually decrease the contrast and readability of actions in notices?

Before:
screen shot 2017-11-16 at 17 24 05

After:
screen shot 2017-11-16 at 17 20 53

If I did not know better, I would not know that there are clickable actions in the new notices :/

Also, I wonder if this dark style does not stand out too much from the rest of Calypso? I could imagine that some users might think it's a popup/ad/whatever, based on how different it feels from the rest of the application (same as when you integrate a third-party widget without changing its style to match the app's style). Might be just me and the usual, human tendency to like the old, familiar stuff ;)

@davewhitley
Copy link
Contributor

Broken icon aligned fixed in #19921

Is it just me or did this change actually decrease the contrast and readability of actions in notices?

2 separate things: The contrast of the notice text, and the recognition of the button.

I suppose the text on the right is a bit harder to recognize as a button. @klimeryk If you'd like, I think you can open an issue and ppl can discuss there.

@klimeryk
Copy link
Contributor

Thanks - will do. I'm a bit surprised that there was not a wider call for testing of this PR - it changed one of the core components of Calypso and it's hard for one person to know all the use cases (and _ab_uses) of notices in Calypso. As a result a few sections showed misaligned notices, as noted in #19910. Additionally, the new notices seem to be optimized for use with an icon - I don't see any examples above without an icon, but they are abundant in Calypso. (don't worry, I'll open issues for all of that ;))

@MichaelArestad
Copy link
Contributor

This doesn't look particularly great on the plugins section. I'm thinking we either need to revise this to be a single color all the way across (maybe with color-coded lights or icons?) or find another solution.

image

@davewhitley
Copy link
Contributor

I think those notices should probably just be text with a colored icon. No background at all. I could also see a single color background (black) working too, no yellow.

@mtias
Copy link
Member

mtias commented Nov 19, 2017

It seems this change should have targeted global notices only first, not inline ones — or at least not compact-inline which has been used to seek a more subdued approach to signaling information and is likely to clash a lot more.

@apeatling
Copy link
Member

@drw158 Are you able to take a look at adjusting inline notices?

@davewhitley
Copy link
Contributor

@apeatling not able to right now, but I made an issue

#20017

@sararosso
Copy link
Contributor

sararosso commented Dec 13, 2017

This change has affected the design of upsells in the sidebar - some wider testing and use cases would have been great to be involved in. We're going to propose some color changes (perhaps a dedicated class so changes like this don't affect them accidentally) for the upsells moving forward and will submit that as a separate issue.

I've created a p2 post on marketingscratchpad about this.

screen shot 2017-12-12 at 7 49 24 am

@rickybanister
Copy link

@mtias could you share what you think would be the best practice for the sidebar notice component—if we didn't want to inherit the global notice style? What's the best way to share some of the structure, but override the component in terms of color or functionality?

aidvu pushed a commit that referenced this pull request Dec 14, 2017
This PR restores the styling of the upgrade banners in the sidebar that were changed as a side effect of #17796.
simison added a commit that referenced this pull request Mar 21, 2018
1) Refactor header to use `Notice` component instead of custom styles:
  - Improves accessibility by following WCAG AA guidelines for contrast ratio.
  - Matches with other Calypso notifications, updated in #17796

2) Use `SectionHeader` component for positioning elements
simison added a commit that referenced this pull request Mar 22, 2018
1) Refactor header to use `Notice` component instead of custom styles:
  - Improves accessibility by following WCAG AA guidelines for contrast ratio.
  - Matches with other Calypso notifications, updated in #17796

2) Use `SectionHeader` component for positioning elements
@simison simison mentioned this pull request Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility (a11y) [Size] M Medium sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.