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

rustdoc: color the question mark operator #37102

Merged
merged 2 commits into from Oct 15, 2016
Merged

rustdoc: color the question mark operator #37102

merged 2 commits into from Oct 15, 2016

Conversation

est31
Copy link
Member

@est31 est31 commented Oct 12, 2016

The idea of coloring ? specially was proposed by @eddyb in: #31436 (comment)

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

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

@est31
Copy link
Member Author

est31 commented Oct 12, 2016 •
edited

How it looks now:
highlight

Please suggest better options if you prefer stronger/other colors.

@est31
Copy link
Member Author

est31 commented Oct 12, 2016

cc @GuillaumeGomez for the case you haven't seen it yet.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 12, 2016 •
edited

I don't like '?'. :'(

:trollface: I thought question mark was considered explicit enough by himself.

@GuillaumeGomez
Copy link
Member

Otherwise it's a nice idea. Just want others' opinion.

cc @steveklabnik

@eddyb
Copy link
Member

eddyb commented Oct 12, 2016

Why not bold + max saturation?

@est31
Copy link
Member Author

est31 commented Oct 12, 2016

Put saturation from 77 to 95 now, and made it bold. Saturation of 100 (#f58300) looks less appealing, I think. The current state:

highlight2

@est31
Copy link
Member Author

est31 commented Oct 12, 2016

I don't like '?'. :'(

Me neither in fact. But decision was to stabilize it, so we have to live with it :/.

This PR intends to highlight ? so that it is more visible, addressing the "? can be overlooked too easily" concern.

@eddyb
Copy link
Member

eddyb commented Oct 12, 2016

It's either the font or the color-abusing scheme that makes it hard to spot. At least bold is slightly better.
(I mean, red for self and Ok?! Anyway I'm not a web designer so I won't comment further)

@est31
Copy link
Member Author

est31 commented Oct 12, 2016

Yes, its far from perfect, but it improves the situation. I don't have much skills in this kind of work either. I have tried changing the background color, but couldn't come up with anything that didn't look totally ugly until I went with normal backgrounds and copied the line number's color for the font.

If nobody has ideas to make it better, changes to make ? easier to spot will still be possible in the future as well.

@GuillaumeGomez
Copy link
Member

@est31: try with #ff9011.

@steveklabnik
Copy link
Member

I think it's a good idea to highlight, I don't have strong feelings about the details.

@GuillaumeGomez
Copy link
Member

New screenshot please. :D

@est31
Copy link
Member Author

est31 commented Oct 12, 2016

highlight3

@GuillaumeGomez
Copy link
Member

Ah nice, a bit better for me.

For now, since it seems that no one has strong feeling about this PR, I'll r+ it. If anyone has issue with it, it'll still be possible to rollback.

@est31: Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Oct 13, 2016

📌 Commit 87cbfb4 has been approved by GuillaumeGomez

@bors
Copy link
Contributor

bors commented Oct 13, 2016

⌛ Testing commit 87cbfb4 with merge e6c01bf...

@bors
Copy link
Contributor

bors commented Oct 13, 2016

💔 Test failed - auto-linux-cross-opt

@eddyb
Copy link
Member

eddyb commented Oct 13, 2016

Okay this is frustrating so I'l speak again: IMO the way the scheme uses bright words for common colors is dumb. It makes it impossible to bring attention to anything when you have a colorfest.
Swapping the orange and the red would probably be a major improvement already.

@est31
Copy link
Member Author

est31 commented Oct 13, 2016

Hmm should that failure be a concern for me?

The error seems to be:

configure: error: in `/buildslave/rust-buildbot/slave/auto-linux-cross-opt/build/obj/powerpc64-unknown-linux-gnu/rt/jemalloc':
configure: error: C compiler cannot create executables

@eddyb
Copy link
Member

eddyb commented Oct 13, 2016

Everything is failing thereb build bots not set up right.

@arielb1
Copy link
Contributor

arielb1 commented Oct 13, 2016

@bors retry

@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 13, 2016
Closed
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Oct 14, 2016
Rollup merge of rust-lang#37102 - est31:rustdoc_question_mark, r=Guil…
3da9ddb 
…laumeGomez

rustdoc: color the question mark operator

The idea of coloring `?` specially was proposed by @eddyb in: rust-lang#31436 (comment)
@sophiajt sophiajt mentioned this pull request Oct 14, 2016
Merged
bors added a commit that referenced this pull request Oct 14, 2016
@bors
Auto merge of #37170 - jonathandturner:rollup, r=jonathandturner
a8d189a 
Rollup of 10 pull requests

- Successful merges: #36307, #36755, #36961, #37102, #37115, #37119, #37122, #37123, #37141, #37159
- Failed merges:
@bors bors merged commit 87cbfb4 into rust-lang:master Oct 15, 2016
@est31 est31 mentioned this pull request Sep 19, 2017
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@steveklabnik steveklabnik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@est31 @rust-highfive @GuillaumeGomez @eddyb @steveklabnik @bors @arielb1