Multiple themes for rustdoc by GuillaumeGomez · Pull Request #47620 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation62 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| @@ -70,6 +70,10 @@ r##" |
|---|
| {sidebar} |
🖌 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This icon is not present in the fonts we bundle with rustdoc, nor in the system fonts in Windows 7:
Some comparisons:
Would be worth doing the color-contrast check on some of these combos like we did a while back when we tweaked the colors the last time. Specifically, in that last one, traits, enums, and primitives seem a little dark compared to the background.
Multiple themes Multiple themes for rustdoc
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have these themes match the mdbook themes, imho
a few tiny comments. glad to see this ship!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since rustdoc is on nightly anyway, why not just use ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make a conversion. I intended to use ? but it cannot be used directly unfortunately...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do the same as ? - it returns an Err instead of None.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also takes the path of the file that caused the failure, which won't be there in the NoneError.
(I also didn't realize that there was a conversion available between NoneError and custom error types >_>)
| // To avoid theme switch latencies as much as possible, we put everything theme related |
|---|
| // at the beginning of the html files into another js file. |
| write(cx.dst.join("theme.js"), format!( |
| r#"var themes = document.getElementById("theme-choices"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be signficantly nicer as an external file that's include_bytes'd.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I need to generate the list of themes which can't be know at compile time. However, I intend to move most of this code into a js file later.
| @@ -70,6 +70,10 @@ r##" |
|---|
| {sidebar} |
🖌 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theme-picker and theme-choices need some ARIA attributes (see this for how it's handled in mdBook).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In mdbook it breaks elements alignment so I'd prefer not use the same mechanisms.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it break elements alignment? Also, this should be a button.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of this being a button... :-/
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you click it? Then it should be a button or a link.
| r#"var themes = document.getElementById("theme-choices"); |
|---|
| var themePicker = document.getElementById("theme-picker"); |
| themePicker.onclick = function() {{ |
| if (themes.style.display === "block") {{ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both branches should update aria-expanded (see mdBook).
| [{}].forEach(function(item) {{ |
|---|
| var div = document.createElement('div'); |
| div.innerHTML = item; |
| div.onclick = function(el) {{ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create an event listener for each theme if I understand correctly. What about just one (on theme-choices)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid js code as much as possible. Like this, no need to try to find which child sent the event.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that comes at a performance cost.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What performance cost? O.o The bottleneck is clearly not in js events.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, not a bottleneck, but why not avoid extra event listeners on a page? It is one or two extra lines of code.
| var currentTheme = document.getElementById("themeStyle"); |
|---|
| var mainTheme = document.getElementById("mainThemeStyle"); |
| [{}].forEach(function(item) {{ |
| var div = document.createElement('div'); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
button, please.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran through a bunch of the colors in the dark theme and suggested lighter variants, based on this color contrast tool. Some colors were unchanged from their main.css counterpart, creating awkward combinations with the new-default text and background colors. In most cases i just used Chrome's color picker tool to tweak each color's lightness value until it was just over a color contrast of 4.5:1 against its chosen background. The result should make things much nicer to read in the dark theme.
| .line-numbers :target { background-color: transparent; } |
|---|
| /* Code highlighting */ |
| pre.rust .kw { color: #8959A8; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 2.77:1, recommend #ab8ac1 instead for 4.88:1.
| /* Code highlighting */ |
| pre.rust .kw { color: #8959A8; } |
| pre.rust .kw-2, pre.rust .prelude-ty { color: #4271AE; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 2.87:1, recommend #769acb instead for 4.96:1.
| pre.rust .kw-2, pre.rust .prelude-ty { color: #4271AE; } |
|---|
| pre.rust .number, pre.rust .string { color: #718C00; } |
| pre.rust .self, pre.rust .bool-val, pre.rust .prelude-val, |
| pre.rust .attribute, pre.rust .attribute .ident { color: #C82829; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 2.59:1, recommend #ee6868 instead for 4.66:1.
| .content span.primitive, .content a.primitive, .block a.current.primitive { color: #2c8093; } |
|---|
| .content span.externcrate, |
| .content span.mod, .content a.mod, .block a.current.mod { color: #967F00; } |
| .content span.trait, .content a.trait, .block a.current.trait { color: #7c5af3; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 2.69:1, recommend #b78cf2 instead for 4.7:1.
| .content .highlighted.primitive { background-color: #9aecff; } |
|---|
| .content span.enum, .content a.enum, .block a.current.enum { color: #508157; } |
| .content span.struct, .content a.struct, .block a.current.struct { color: #df3600; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 2.72:1, recommend #ff794d instead for 4.73:1.
| .content .highlighted.foreigntype { background-color: #f5c4ff; } |
|---|
| .content .highlighted.macro { background-color: #8ce488; } |
| .content .highlighted.constant, |
| .content .highlighted.static { background-color: #c3e0ff; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio of 1.17:1, recommend #0063cc instead for 4.95:1.
| .content .highlighted.method, |
|---|
| .content .highlighted.tymethod { background-color: #4950ed; } |
| .content .highlighted.type { background-color: #38902c; } |
| .content .highlighted.foreigntype { background-color: #f5c4ff; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 1.27:1, recommend #b200d6 instead for 4.68:1.
| .content .highlighted a, .content .highlighted span { color: #eee !important; } |
|---|
| .content .highlighted.trait { background-color: #013191; } |
| .content .highlighted.mod { background-color: #803a1b; } |
| .content .highlighted.externcrate { background-color: #afc6e4; } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio 1.51:1, recommend #396bac instead for 4.67:1.
| } |
|---|
| #help > div { |
| background: #e9e9e9; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color contrast ratio of 1.12:1, recommend #4d4d4d instead for 6.22:1.
| #help dt { |
| border-color: #bfbfbf; |
| background: #fff; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the standard #dddddd for the text, this gives 1.36:1. You can either change this background to #4d4d4d (same as the help div background), or add a color: black; to this block to make it the same as the light-theme ones.
(After a conversation on IRC, imperio suggested that i just push the suggested changes to his branch, so i've done just that.)
After some discussion on IRC about how to make the theme stick between page loads without flashing a white background each time, we've settled on something i'm willing to accept! This feature is a long time coming, and something that's been requested a lot, so i'm excited to get this rolling.
@bors r+ p=1
(the extensive changes to css/js means that anything else that touches any of that has a merge conflict with this >_>)
📌 Commit 5b85044 has been approved by QuietMisdreavus
⌛ Testing commit 5b85044 with merge 5032e43715dcc3167d88f215c408594d9128dd3a...
@bors retry
All tests passed but bors didn't notice it.
bors added a commit that referenced this pull request
Does doc.rust-lang.org/nightly rebuild with every nightly? Will I be able to enjoy those sweet sweet dark colors tomorrow already?!
Yup! Those nightly docs are updated with every nightly, so by the time the next nightly comes up, you can enjoy that dark theme on the standard library docs, at least. :D
So, the change is now live: https://doc.rust-lang.org/nightly/std/mem/. However, it seems to default to the dark theme, and clicking to change it does nothing in Firefox? Seems to work fine on Chrome though. That is, I can click the style button, but not dark/main to switch.
Ah, that's another issue hehe. There is another opened here. Funny that firefox has this much issues...
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…anup, r=GuillaumeGomez
rustdoc: remove unused ID mainThemeStyle
This was added in rust-lang#47620 and used to build the URL of the theme stylesheets. It isn't used any more, because rust-lang#101702 changed it so that the URL was supplied in a <meta> tag, which also provides the hashes of the files.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#115655 - notriddle:notriddle/rustdoc-fe-cleanup, r=GuillaumeGomez
rustdoc: remove unused ID mainThemeStyle
This was added in rust-lang#47620 and used to build the URL of the theme stylesheets. It isn't used any more, because rust-lang#101702 changed it so that the URL was supplied in a <meta> tag, which also provides the hashes of the files.
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.