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 green to pass 4.5:1 #30972

Closed
wants to merge 2 commits into from
Closed

Update green to pass 4.5:1 #30972

wants to merge 2 commits into from

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented Jun 5, 2020

On the road to #30548, and as mentionned in #30468, our current $green is not contrasted enough.

My first attempt was too far from other colors, so here's another one (and this PR will only try those greens). I tried to keep consistency by using the current .btn-success hover color, so this is basically the current green darkened from 7.5%.

Preview in color swatchesPreview in buttons

@ffoodd ffoodd requested review from mdo and MartijnCuppens June 5, 2020 14:43
@ffoodd ffoodd marked this pull request as ready for review June 8, 2020 12:49
@ffoodd ffoodd requested a review from a team as a code owner June 8, 2020 12:49
@mdo
Copy link
Member

mdo commented Jun 8, 2020

It's so dark. 😭

Is this the bare minimum to hit 4.5, or does this go past it a bit? I'd like to take as much as I can on the lightness/vibrancy of it since green is such a difficult color to get right.

@ffoodd
Copy link
Member Author

ffoodd commented Jun 8, 2020

I'll try to suggest a few other greens. 4.5 is the minimum indeed, but playing with hue or saturation it should be possible to get a lighter green.

I only used this as a startong point, this is the simpler variant to try since we already have it 😄

@MartijnCuppens
Copy link
Member

It's so dark. 😭

Yeah, I agree, the button looks like it's pressed. Here are some alternative colors, but as expected, they are all darker: https://contrast-finder.tanaguru.com/result.html?foreground=%23FFFFFF&background=%2328A745&ratio=4.5&isBackgroundTested=true&algo=Rgb&distanceSort=asc

@ffoodd
Copy link
Member Author

ffoodd commented Jun 8, 2020

BTW, is the white text color a requirement? Switching to dark or black would allow us to keep existing green.

@mdo
Copy link
Member

mdo commented Jun 9, 2020

BTW, is the white text color a requirement? Switching to dark or black would allow us to keep existing green.

Kinda? I like the consistency but I’d be open to dark text since we have that for our warning buttons.

@ffoodd
Copy link
Member Author

ffoodd commented Jun 9, 2020

I'm trying another green: #0a8a1d (vs #28a745) which has a higher saturation than my previous try, #218838.

FWIW, here's a great link to see how complicated it is to improve the current green: https://color.review/check/28A745-FFFFFF

Either playing with, hue, sturation or lightness gives us weird greens.

FYI to get dark text on success, it'll require to increase the $min-contrast-ratio:

  1. it'll also change .btn-info color
  2. it has to increase from a pretty large amount, to ensure hover keeps the same text color.
  3. and in fact this should be the end, with Ensure to pass 4.5:1 contrast ratio everywhere #30548

One thing to consider is also to switch between darken() or lighten() for hover & active states, depending on the contrast color (PR incoming).

This makes sense for every hoverable colored background, I guess.

@ffoodd
Copy link
Member Author

ffoodd commented Jun 9, 2020

Still thinking out loud: if #30989 is accepted, then changing the green might not be valuable anymore.

The issue in #30468 was that foreground color changed on hover: that was because we always darkened the background, and got the foreground by using color-contrast(). With that in mind, if we try to bump $min-contrast-ratio to 4.5, this wouldn't happen anymore (however success and info buttons would have dark text color).

@mdo if this makes sense, we should consider to simply close this one and keep the current green and cyan. Just notice that the current green with a dark text color is not very happy, IMHO. We may end up lightening it :D

@ffoodd ffoodd added the on-hold label Jun 9, 2020
@mdo mdo changed the base branch from master to main June 16, 2020 19:32
@ffoodd
Copy link
Member Author

ffoodd commented Jul 15, 2020

Closing this since it's tackled with #31276 🎉

@ffoodd ffoodd closed this Jul 15, 2020
@XhmikosR XhmikosR deleted the master-fod-green-update branch November 11, 2020 08:06
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.

4 participants