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

Migrate CSS highlight to CSS variables #102663

Closed

Conversation

GuillaumeGomez
Copy link
Member

Part of #98460.

There are UI changes: I unified the colors as follows:

  • All keyword elements now have the same color.
  • All the literal elements now have the same color.
  • I removed the italic on .self in the ayu theme as it was the only theme with it.

You can test it here.

cc @jsha
r? @notriddle

@GuillaumeGomez GuillaumeGomez added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Oct 4, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 4, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

A change occurred in the Ayu theme.

cc @Cldfire

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2022
@notriddle
Copy link
Contributor

I don't think the color changes are an improvement:

  • Prelude items are not keywords. You can shadow them, they're part of the regular grammar, and many of them are plain library code with no special lang-item powers. Implying that None and fn are in some way the same thing seems like it would confuse beginners.

  • Is making all keywords the same color actually a positive? It seems like the old color scheme intentionally made &, *, and mut their own color, separate from all other "keywords", because they're so heavily used. Making them the same color means, in function signatures, half the code is getting highlighted with the same shade of purple, which mostly defeats the purpose of syntax highlighting.

    image

  • If I was going to tweak the way literal expressions are highlighted, I'd make bool literals and numeric literals the same color, while giving string literals a separate color. Strings are special, because they can be arbitrarily long and contain text that looks like syntax ("false" and false are both valid), so it makes sense to want to distinguish them.

@GuillaumeGomez
Copy link
Member Author

I'll update following your suggestions then to see if it looks better.

@GuillaumeGomez
Copy link
Member Author

I updated the style as you recommended. I tried to pick good colours for literals but don't hesitate if you have better ones. :)

@bors
Copy link
Contributor

bors commented Oct 6, 2022

☔ The latest upstream changes (presumably #102741) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict.

@notriddle
Copy link
Contributor

@jsha
Copy link
Contributor

jsha commented Oct 10, 2022

In general we should not combine refactorings with nontrivial feature changes. Can you make the refactoring (change to CSS variables) without making the feature change (merging colors)? Then we can discuss the feature change independently.

@GuillaumeGomez
Copy link
Member Author

It's a bit complicated because ayu combines colors differently than the other themes, hence why I tried to clarify things a bit. I'll try to keep the current behaviour as much as possible then.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 14, 2022
…ht-without-change, r=notriddle

Migrate css highlight without change

This is a "previous" version of rust-lang#102663: only migrating to CSS variables, no changes. It's a bit more verbose because rules are not coherent between themes.

r? `@notriddle`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 14, 2022
…ht-without-change, r=notriddle

Migrate css highlight without change

This is a "previous" version of rust-lang#102663: only migrating to CSS variables, no changes. It's a bit more verbose because rules are not coherent between themes.

r? ``@notriddle``
albertlarsan68 added a commit to albertlarsan68/rust that referenced this pull request Oct 14, 2022
…ht-without-change, r=notriddle

Migrate css highlight without change

This is a "previous" version of rust-lang#102663: only migrating to CSS variables, no changes. It's a bit more verbose because rules are not coherent between themes.

r? ```@notriddle```
@notriddle notriddle closed this Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants