Multiple themes for rustdoc #47620
Multiple themes for rustdoc #47620
Conversation
GuillaumeGomez
force-pushed
the
multiple-themes
branch
from
9e281c2 to
9aee164
Compare
src/librustdoc/html/layout.rs
Outdated
| @@ -70,6 +70,10 @@ r##"<!DOCTYPE html> | |||
| {sidebar} | |||
| </nav> | |||
|
|
|||
| <div id="theme-picker">🖌 | |||
There was a problem hiding this comment.
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. |
| $file)) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
since rustdoc is on nightly anyway, why not just use ?
There was a problem hiding this comment.
We need to make a conversion. I intended to use ? but it cannot be used directly unfortunately...
There was a problem hiding this comment.
This doesn't do the same as ? - it returns an Err instead of None.
There was a problem hiding this comment.
isn't that how ? works on an Option inside something returning Result? You get https://doc.rust-lang.org/stable/std/option/struct.NoneError.html, and you can do the appropriate From impl if you want a custom thing
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
this might be signficantly nicer as an external file that's include_bytes'd.
There was a problem hiding this comment.
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.
src/librustdoc/html/layout.rs
Outdated
| @@ -70,6 +70,10 @@ r##"<!DOCTYPE html> | |||
| {sidebar} | |||
| </nav> | |||
|
|
|||
| <div id="theme-picker">🖌 | |||
There was a problem hiding this comment.
theme-picker and theme-choices need some ARIA attributes (see this for how it's handled in mdBook).
There was a problem hiding this comment.
In mdbook it breaks elements alignment so I'd prefer not use the same mechanisms.
There was a problem hiding this comment.
How does it break elements alignment? Also, this should be a button.
There was a problem hiding this comment.
I don't see the point of this being a button... :-/
There was a problem hiding this comment.
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") {{ |
There was a problem hiding this comment.
Both branches should update aria-expanded (see mdBook).
src/librustdoc/html/render.rs
Outdated
| [{}].forEach(function(item) {{ | ||
| var div = document.createElement('div'); | ||
| div.innerHTML = item; | ||
| div.onclick = function(el) {{ |
There was a problem hiding this comment.
This will create an event listener for each theme if I understand correctly. What about just one (on theme-choices)?
There was a problem hiding this comment.
I prefer to avoid js code as much as possible. Like this, no need to try to find which child sent the event.
There was a problem hiding this comment.
Yeah, but that comes at a performance cost.
There was a problem hiding this comment.
What performance cost? O.o The bottleneck is clearly not in js events.
There was a problem hiding this comment.
True, not a bottleneck, but why not avoid extra event listeners on a page? It is one or two extra lines of code.
src/librustdoc/html/render.rs
Outdated
| var currentTheme = document.getElementById("themeStyle"); | ||
| var mainTheme = document.getElementById("mainThemeStyle"); | ||
| [{}].forEach(function(item) {{ | ||
| var div = document.createElement('div'); |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
Color contrast ratio 1.51:1, recommend #396bac instead for 4.67:1.
| } | ||
|
|
||
| #help > div { | ||
| background: #e9e9e9; |
There was a problem hiding this comment.
Color contrast ratio of 1.12:1, recommend #4d4d4d instead for 6.22:1.
|
|
||
| #help dt { | ||
| border-color: #bfbfbf; | ||
| background: #fff; |
There was a problem hiding this comment.
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.) |
carols10cents
added
the
S-waiting-on-author
|
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 |
|
⌛ Testing commit 5b85044 with merge 5032e43715dcc3167d88f215c408594d9128dd3a... |
|
💥 Test timed out |
|
@bors retry All tests passed but bors didn't notice it. |
|
Ah! That's interesting! |
Multiple themes for rustdoc r? @QuietMisdreavus
|
☀️ Test successful - status-appveyor, status-travis |
|
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... |
…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.
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.
r? @QuietMisdreavus