-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Add resource-suffix option for rustdoc #48511
Add resource-suffix option for rustdoc #48511
Conversation
Hi @GuillaumeGomez this is great and it will fix docs.rs breakage and save me from a lot of trouble in future. Only javascript portion of theme picker needs some adjustments now.
Some example I used to make theme-picker work with
function switchTheme(styleElem, mainStyleElem, newTheme) {
styleElem.href = mainStyleElem.href.replace("rustdoc-EXAMPLE.css", newTheme + ".css");
updateLocalStorage('theme', newTheme);
}
["dark-EXAMPLE","main-EXAMPLE"].forEach(function(item) {
var but = document.createElement('button');
but.innerHTML = item;
but.onclick = function(el) {
switchTheme(currentTheme, mainTheme, item);
};
themes.appendChild(but);
}); |
17c234c
to
2497447
Compare
Apparently it's all good for @onur. So therefore, I suppose it can get merged. cc @QuietMisdreavus |
Yes it's all good @GuillaumeGomez. I have some (most likely) unrelated tests failing on docs.rs. Trying to fix them. Edit: fixed them, they are not related to this. |
Perfect, now we just need to hear back from @QuietMisdreavus! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me a minute to figure out how the names would get displayed to the users in the theme picker. Since the resource suffix is added into the JS file separately, the theme picker names should stay the same (i.e. without the suffix) so i only have a few comments before i'm willing to sign off on this.
try_err!(fs::copy(css, out), css); | ||
} | ||
write(cx.dst.join("normalize.css"), | ||
write(cx.dst.join(&format!("normalize{}.css", cx.shared.resource_suffix)), | ||
include_bytes!("static/normalize.css"))?; | ||
write(cx.dst.join("FiraSans-Regular.woff"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing the fonts and their copyright notices aren't relevant for wanting to switch the filenames out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @onur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @onur from a conversation on IRC, it matters less for the fonts to handle it, since they're loaded from the CSS (which would need to also be processed to use the suffix), and they don't churn like the CSS/JS do. This is fine as-is.
{ | ||
let mut data = format!("var resourcesSuffix = \"{}\";\n", | ||
cx.shared.resource_suffix).into_bytes(); | ||
data.extend_from_slice(include_bytes!("static/storage.js")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a Vec<u8>
and include_bytes
here instead of a String
and include_str
seems weird to me. I get that on the very next line it needs to be bytes for the write
call, but i don't know how much we gain by sidesteppning the UTF-8 check on this extend call.
src/librustdoc/lib.rs
Outdated
@@ -261,6 +261,11 @@ pub fn opts() -> Vec<RustcOptGroup> { | |||
"check if given theme is valid", | |||
"FILES") | |||
}), | |||
unstable("resource-suffix", |o| { | |||
o.optopt("", | |||
"resource-suffix", "suffix which will be added at the end of resource files", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd, both in verb tense and what we mean by "resource files". What about this?
suffix to add to CSS and JavaScript files, e.g. "main.css" will become "main-suffix.css"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It changed through the writing of this PR. Now we have an almost definitive version so it's more clear. I'll update to your sentence.
2497447
to
831009f
Compare
r=me pending travis |
@bors: r=QuietMisdreavus |
📌 Commit 831009f has been approved by |
…fix, r=QuietMisdreavus Add resource-suffix option for rustdoc Alternative version of rust-lang#48442. cc @onur r? @QuietMisdreavus
…fix, r=QuietMisdreavus Add resource-suffix option for rustdoc Alternative version of rust-lang#48442. cc @onur r? @QuietMisdreavus
…fix, r=QuietMisdreavus Add resource-suffix option for rustdoc Alternative version of rust-lang#48442. cc @onur r? @QuietMisdreavus
⌛ Testing commit 831009f with merge a012ad893062d107b305274a20d56c3a75c25bac... |
@bors: retry prioritizing rollup with this in it |
Alternative version of #48442.
cc @onur
r? @QuietMisdreavus