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.

Sign up for GitHub

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

Jump to bottom

Changed color of struct link from #ff794d to #2dbfb8 for Rust docs #47806

Merged
merged 2 commits into from
Feb 15, 2018
Merged

Changed color of struct link from #ff794d to #2dbfb8 for Rust docs #47806

merged 2 commits into from
Feb 15, 2018

Conversation

PramodBisht
Copy link
Contributor

@PramodBisht PramodBisht commented Jan 27, 2018 •
edited by kennytm
Loading

This is in reference to #47801

here I have changed the default color of struct link for #ff794d to #2dbfb8

cc: @nagisa @timClicks

@rust-highfive
Copy link
Contributor

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@GuillaumeGomez
Copy link
Member

All struct are red colored, even in doc examples. I'm not sure this is a very good idea to change it... Also, red doesn't mean that a resource isn't available. At least I don't feel it like this...

@PramodBisht
Copy link
Contributor Author

@GuillaumeGomez, @QuietMisdreavus,
Please feel free to decline PR, if you both think it's not good idea :)

@kennytm kennytm added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2018
@nagisa
Copy link
Member

nagisa commented Jan 27, 2018

This is what the currently proposed colours look like:

screenshot from 2018-01-27 20-15-00
screenshot from 2018-01-27 20-14-25

@GuillaumeGomez would you be fine with changing the colour to some hue that is less far away from red? e.g. something like

screenshot from 2018-01-27 20-18-35

or

screenshot from 2018-01-27 20-18-08

@nagisa
Copy link
Member

nagisa commented Jan 27, 2018 •
edited
Loading

Note, there is precedent for changing the colour already. We quite recently changed the lightness of many link colours to make them darker.

@GuillaumeGomez
Copy link
Member

The orange seems fine. However, it's more of a personal taste here. Also, it'll be need to be changed in the dark theme as well (which wasn't the case in this PR).

@nagisa
Copy link
Member

nagisa commented Jan 29, 2018 via email

@GuillaumeGomez
Copy link
Member

It puts the same color than in the main theme. Seems strange that two different themes get one color in common. Maybe it renders just fine, but seems weird.

@QuietMisdreavus
Copy link
Member

As-is, the proposed color is too light in the main theme. If you want to keep the teal for it, change the main.css color to #1b7e6e instead. That will give a much better color contrast of 4.93:1.

@PramodBisht
Copy link
Contributor Author

@QuietMisdreavus would not #1b7e6e look much similar to color or macro(#68000).

@QuietMisdreavus
Copy link
Member

I guess so. I think the pink color in @nagisa's comment is good - the orange seems like it might be too light, and might also be too close to the existing orange of type aliases.

@PramodBisht
Copy link
Contributor Author

PramodBisht commented Jan 31, 2018 •
edited
Loading

@QuietMisdreavus @GuillaumeGomez @nagisa Pink look good on white theme. Pushed code change as well.

@kennytm
Copy link
Member

kennytm commented Jan 31, 2018 •
edited
Loading

@nagisa's pink is #df00b2, which gives 4.39:1 ratio. #dc00b0 would be exactly 4.5:1.

Note that foreign type uses a similar pink already #cd00e2 (4.47:1 😓, 4.5:1 would be #cc00e1), although it is more purplish.

(The new code uses #ad448e.)

@GuillaumeGomez
Copy link
Member

Not orange then? 😢

@kennytm
Copy link
Member

kennytm commented Jan 31, 2018 •
edited
Loading

Current color of type alias is already orange #ba5d00.

Nagisa's orange is #df8e00 which is too bright. Pushing to 4.5:1 gives #a76900.

@GuillaumeGomez
Copy link
Member

Damn. I found the red really good enough, that's too bad...

@pietroalbini
Copy link
Member

@GuillaumeGomez ping! What's the status of this PR?

@GuillaumeGomez
Copy link
Member

We still need to have a consensus over the color.

@kennytm kennytm added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2018
@kennytm
Copy link
Member

kennytm commented Feb 5, 2018

Nominating to docs team in this case 😛

Current choices on table:

  • No change = Red (#df3600)
  • This PR = Purple (#ad448e)
  • Pink (#dc00b0) (may be too close to extern type #cd00e2)
  • Brown (#a76900) (may be too close to type alias #ba5d00)
  • Orange (#df8e00) (fails the 4.5:1 contrast ratio)

Existing colors:

Kind main.css dark.css
Macro #068000 #09bd00
Enum #508157 #82b089
Primitive #2c8093 #43aec7
Module #4d76ae #bda000
Static #546e8a #82a5c9
Trait #7c5af3 #b78cf2
Extern type #cd00e2 #dd7de8
Union #767b27 #a6ae37
Function #9a6e31 #2BAB63
Type alias #ba5d00 #ff7f00
Struct (current) #df3600 #ff794d

@QuietMisdreavus QuietMisdreavus mentioned this pull request Feb 8, 2018
Closed
@PramodBisht
Copy link
Contributor Author

@QuietMisdreavus @kennytm @GuillaumeGomez @nagisa what next?

@kennytm
Copy link
Member

kennytm commented Feb 13, 2018

@PramodBisht As linked from rust-docs/team#5, it is on doc team's agenda in the coming meeting in 9 hours (Feb 13th, 20:00 UTC). You may join the IRC channel #rust-docs on irc.mozilla.org at that time.

@steveklabnik
Copy link
Member

We talked about it in the docs team meeting today, and the inclination is to merge. In the future, we may want to change structs, enums, and unions to have similar colors, but that's a big change, and fixing the immediate issue is better than bogging down this PR in sorting that out.

@PramodBisht thank you so much!

@bors: r+ rollup

@bors
Copy link
Collaborator

bors commented Feb 13, 2018

📌 Commit 434bfb1 has been approved by steveklabnik

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 13, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
@kennytm
Rollup merge of rust-lang#47806 - PramodBisht:feature/47801, r=stevek…
c89976e 
…labnik

Changed color of struct link from #ff794d to #2dbfb8 for Rust docs

This is in reference to rust-lang#47801

here I have changed the default color of struct link for `#ff794d` to `#2dbfb8`

cc: @nagisa  @timClicks
@kennytm kennytm mentioned this pull request Feb 14, 2018
Merged
bors added a commit that referenced this pull request Feb 14, 2018
@bors
Auto merge of #48203 - kennytm:rollup, r=kennytm
d3fb063 
Rollup of 22 pull requests

- Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035
- Failed merges:
bors added a commit that referenced this pull request Feb 15, 2018
@bors
Auto merge of #48203 - kennytm:rollup, r=kennytm
f6ee1ec 
Rollup of 22 pull requests

- Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035
- Failed merges:
bors added a commit that referenced this pull request Feb 15, 2018
@bors
Auto merge of #48203 - kennytm:rollup, r=kennytm
1670a53 
Rollup of 23 pull requests

- Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035, #48210
- Failed merges:
@kennytm kennytm merged commit 434bfb1 into rust-lang:master Feb 15, 2018
@PramodBisht PramodBisht deleted the feature/47801 branch March 7, 2018 16:23
@steveklabnik steveklabnik mentioned this pull request May 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveklabnik steveklabnik steveklabnik approved these changes

@QuietMisdreavus QuietMisdreavus Awaiting requested review from QuietMisdreavus

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@PramodBisht @rust-highfive @GuillaumeGomez @nagisa @QuietMisdreavus @kennytm @pietroalbini @steveklabnik @bors