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

Union const colors #43558

Merged
merged 6 commits into from
Aug 7, 2017
Merged

Union const colors #43558

merged 6 commits into from
Aug 7, 2017

Conversation

GuillaumeGomez
Copy link
Member

Fixes #43523

What do you think of these colors:

screen shot 2017-07-30 at 15 10 57

?

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@GuillaumeGomez
Copy link
Member Author

cc @rust-lang/docs

@QuietMisdreavus
Copy link
Member

The "Union" could stand to be a bit darker, but it also feels about as light as the primitive color, so i'll let it fly.

@TedDriggs - you wanted to check out the new colors, this is the PR that adds them.

@carols10cents
Copy link
Member

Red/green colorblindness is the most common kind, so structs and enums might not be distinguishable from each other. I have a friend who has some colorblindness, would you like me to run this by him?

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2017
@GuillaumeGomez
Copy link
Member Author

@carols10cents: It'd be very helpful! Maybe he can suggests better colors as well? :)

@carols10cents
Copy link
Member

Maybe he can suggests better colors as well? :)

It's quite likely! Asking!

@carols10cents
Copy link
Member

Ok! I sent my friend the screenshot and this is what he has to say:

Struct & Trait are visually distinct. CONST and Union are hard to discern for me as text colors. Enum is fine compared to Struct and Trait, but gets a little lost to me next to Union or CONST.

Is this syntax highlighting for the documentation? If so, I'd recommend stronger contrast all around if they're meant to sit on a light background.

Neither CONST nor Union pass WCAG contrast guidelines for use against a white background (http://leaverou.github.io/contrast-ratio/#rgb%28200%2C153%2C89%29-on-white and http://leaverou.github.io/contrast-ratio/#rgb%28193%2C200%2C59-on-white).

Enum is cutting it close too (http://leaverou.github.io/contrast-ratio/#rgb%28105%2C148%2C94%29-on-white). Upping contrast definitely helps me distinguish colors.

I hope this helps. I'll load the colors into the contrast ratio tool and mess around with them to see if I can come up with something I think looks good and is easily distinguishable for me at small sizes against white. :)

@arielb1
Copy link
Contributor

arielb1 commented Aug 1, 2017

r? @QuietMisdreavus (or someone else on the docs team)

@jnf
Copy link

jnf commented Aug 2, 2017

Hello! I'm the friend @carols10cents referenced above. I have (relatively mild) colorblindness, primarily in distinguishing warm tones. I put together a Code Pen with some contrast comparisons and an alternate palette. I don't have any context for how these colors are used (my guess is syntax highlighting for docs), so please forgive any misuse.

Anyway, thank you Carol for asking. Yinz keep being this awesome and considerate and I may just have to learn some Rust. ^_^

@GuillaumeGomez
Copy link
Member Author

I'll take a look and try to make it easier for colorblind people to see the difference.

@kennytm
Copy link
Member

kennytm commented Aug 2, 2017

The complete list of colors, as the Code Pen only listed five. CR=4.5-ing done via http://accessible-colors.com/. Just raising the contrast should not be enough, if the CR of every color is 4.5 they would all become the same shade of gray when the hue is washed out.

Kind Original color After CR=4.5'ed
Struct #df3600 #df3600
Typedef #e57300 #df3600
Const (this PR) #c7944f #9a6e31
Function #8c6067 #8c6067
Union (this PR) #c0c74f #767b27
Enum #5e9766 #508157
Macro #068000 #068000
Primitive #39a7bf #2C8093
Module #4d76ae #4d76ae
Trait #7c5af3 #7c5af3
(Coblis simulation of above)

1

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 2, 2017

Here is the new output:

screen shot 2017-08-02 at 23 08 57

Is it fine or is there something else I can do?

@steveklabnik
Copy link
Member

As long as the contrast is high enough, I'm 👍

@QuietMisdreavus
Copy link
Member

Looks good to me! I'd like to hear from the others in this thread first, though.

@carols10cents
Copy link
Member

I defer entirely to @jnf (who has a non-Rust real job in PST so will likely comment in a few hours at the earliest, thank you again for your time on this Jeremy!!!)

@kennytm
Copy link
Member

kennytm commented Aug 3, 2017

@GuillaumeGomez Could we keep functions brown and assign the blue to constants or unions? Existing colors should better be kept "stable".

@GuillaumeGomez
Copy link
Member Author

Ok, switched functions and constants color:

screen shot 2017-08-03 at 12 46 42

@ollie27
Copy link
Member

ollie27 commented Aug 3, 2017

What about statics? There are also structfield, variant, associatedtype and associatedconstant.

For all of these the background highlight color on the search page will also need to be updated to match:

.content .highlighted.trait { background-color: #fece7e; }
.content .highlighted.mod { background-color: #afc6e4; }
.content .highlighted.enum { background-color: #b4d1b9; }
.content .highlighted.struct { background-color: #e7b1a0; }
.content .highlighted.fn { background-color: #c6afb3; }
.content .highlighted.method { background-color: #c6afb3; }
.content .highlighted.tymethod { background-color: #c6afb3; }
.content .highlighted.type { background-color: #c6afb3; }
.

@GuillaumeGomez
Copy link
Member Author

@ollie27: Forgot about them. I update as soon as I can!

@GuillaumeGomez
Copy link
Member Author

Updated!

screen shot 2017-08-03 at 22 55 04

@GuillaumeGomez
Copy link
Member Author

Anything else to do in here?

@ollie27
Copy link
Member

ollie27 commented Aug 5, 2017

Any reason not to add colors for statics? After this they'll be the only item type that gets a full page but no colors.

The background highlight colors for trait and type are still wrong but I guess that's a pre-existing bug. There are still no background colors for macro, constant, primitive, externcrate and static.

@GuillaumeGomez
Copy link
Member Author

@ollie27: Fixed as well! Anything else?

@ollie27
Copy link
Member

ollie27 commented Aug 6, 2017

Looks good to me 👍

@GuillaumeGomez
Copy link
Member Author

Since you were the last one having issues, I suppose we can r+?

@QuietMisdreavus
Copy link
Member

It sounds like any interested party is okay with what's left, or was willing to defer to someone else. Let's get this out the door. If anyone finds any more issues with contrast, open another issue and we can work it out!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 7, 2017

📌 Commit 00b362e has been approved by QuietMisdreavus

@bors
Copy link
Contributor

bors commented Aug 7, 2017

⌛ Testing commit 00b362e with merge 0188ec6...

bors added a commit that referenced this pull request Aug 7, 2017
…reavus

Union const colors

Fixes #43523

What do you think of these colors:

<img width="1440" alt="screen shot 2017-07-30 at 15 10 57" src="https://user-images.githubusercontent.com/3050060/28753752-6b175a22-7539-11e7-978e-949f3a947d18.png">

?
@bors
Copy link
Contributor

bors commented Aug 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 0188ec6 to master...

@bors bors merged commit 00b362e into rust-lang:master Aug 7, 2017
@GuillaumeGomez GuillaumeGomez deleted the union-const-colors branch August 7, 2017 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants