-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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 node-green to pass WCAG AA #536
Conversation
Based on the new colors defined with the updated logo, this is the the closest green that passes on a white background for small text where a contrast ratio of 4.5 is required.
The big buttons seem a little dark. I'm worried about them looking actionable enough, maybe a slightly brighter green and the green in this screenshot as the rollover color? |
Home page updated for aesthetics, download page updated for contrast
Thanks @mikeal - updated the buttons, I agree they looked dull. I shifted the rollover color a bit based on the default state. Also had to update the download page a bit since the gray background during hover made the new node-green nearly unreadable. |
i actually prefer the first set of button colors :X the second set looks a bit too much like |
I'll let this stand until at least tomorrow to maybe get some broader consensus. Thanks. |
@jrit thank you for doing this 😄 |
I generally like the new colours. Thanks @jrit :) |
I prefer the brighter ones in your updated version |
Sounds like 2 votes for bright buttons and one for dull. I'm leaning toward the brighter one as well. I'm on standby if anyone has any further requirements before this can be merged. |
we should wait for @mikeal to respond since he was the one that originally asked for the color change :P |
Woot, the website's colors have felt just not right for like a week now since the new logo haha. :P |
@@ -2,7 +2,8 @@ $body-max-width = 980px | |||
|
|||
$white = #fff | |||
|
|||
$node-green = #80bd01 | |||
$node-green = #43853d | |||
$bright-green = #028a00 | |||
$light-green = #f1fbda | |||
$hover-green = #d9ebb3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these two be updated with the new color guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at where/how they are being used
makes on/hover states more consistent and fixes contrast on light backgrounds
Thanks all for your feedback. The suggestion to consolidate a few styles led me to consolidate some of the visuals as well so the active/hover/on states feel more consistent. I also fixed the contrast of green text on the light-green background. One other very minor change is adjusting the line-height and padding on the left navigation so when an item wraps, like "Technical Governance" it doesn't look like it is two separate links. Arrows on the below indicate hover states. |
Damn, I totally missed this PR. I almost filed my own PR with these changes: -$node-green = #80bd01
-$light-green = #f1fbda
-$hover-green = #d9ebb3
+$node-green = #44883e
+$light-green = #e4f2e3
+$hover-green = #c0e0bd And the download button hover: &:hover
- background-color saturation($node-green, 80%)
+ background-color saturation($node-green, 60%) What you might want to adapt is my |
Oh, and disregard me, I should've read the changes first. Overall LGTM, except those platform logos which are still the old color. I'd convert these to SVG, but that can likely happen at a later point in time. |
$node-green = #80bd01 | ||
$light-green = #f1fbda | ||
$hover-green = #d9ebb3 | ||
$node-green = #43853d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not #44883e
(http://rgb.to/pantone/7741-c)? I'd say it's good enough contrast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was the original spec color (correct me if I'm wrong) and it turned out to just barely not pass WCAG, which was actually the impetus for this PR originally. It is my hope in the future we can include WCAG validation as part of the site build process, so even the minor difference would cause a failure. Just taking care of the most important contrast differences for now, baby steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, It's barely noticeable, so I don't have much of an opinion, but I'd lean on more the side of using the original color than to make it match the guideline.
@silverwind I agree about the platform icons on both color and file format. If you want to open a PR for that I bet it could just follow on this one being merged. |
Filed #548 for SVG icons, they use |
And those SVGs are in master. I don't think you need to change anything in your PR, but best to double-check. |
@silverwind made a small update to fill SVGs with white on hover as they were the same color as the background otherwise. Should be good now. |
update node-green to pass WCAG AA
Okay, merged. Should have squashed to commits in hindsight, hope it's fine. |
Thank you! |
I thought maybe https://nodejs.org/dist/latest-v5.x/docs/api/ would get updated, but that CSS lives somewhere else I guess. Do you know where that needs to be changed @silverwind ? |
API is handled here: https://github.com/nodejs/node/blob/master/doc/api_assets/style.css I'm preparing a pull request for it right now, but it will only get live on next release of each branch, unfortunately. |
👍 |
The API docs are still using the old theme. |
@stevemao see a couple comments above yours |
Based on the new colors defined with the updated logo,
this is the the closest green that passes on a white
background for small text where a contrast ratio
of 4.5 is required.
Resolves #501
Here are some previews of how this will look: