Uh oh!
There was an error while loading. Please reload this page.
Add rustdoc settings menu - #49954
Conversation
rust-highfive
commented
Apr 13, 2018
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
a79eaef to
0d222acCompareGuillaumeGomez
commented
Apr 15, 2018
Ready it is! |
ollie27
commented
Apr 17, 2018
If <ahref='./std_unicode/index.html'><imgsrc='https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png' alt='logo' width='100'></a>Perhaps it would be better for the settings to be in a drop-down menu like the themes instead of a separate file. |
QuietMisdreavus
commented
Apr 17, 2018
Alternately, if it's going to be in a separate page, is it possible to put it into On the other hand, when i ran it myself, the link gets changed to |
bors
commented
Apr 17, 2018
☔ The latest upstream changes (presumably #50033) made this pull request unmergeable. Please resolve the merge conflicts. |
ollie27
commented
Apr 17, 2018
Another option would be to use a pop-up like the help pop-up. |
GuillaumeGomez
commented
Apr 17, 2018
@ollie27: Check out the changes, I remove the url on the logo. :) |
0d222ac to
f2ad3c3CompareGuillaumeGomez
commented
Apr 17, 2018
And I'd prefer to avoid adding even more JS than necessary. Settings is in its own page, with its own JS and everything is fine as is. |
ollie27
commented
Apr 17, 2018
The issue with removing the url with JS is that it doesn't help with bit-for-bit deterministic builds (#34902). Also the logo itself is crate specific, as is the favicon. How about not sharing settings.html and generating it once for each crate next to all.html? |
GuillaumeGomez
commented
Apr 18, 2018
Because that's repeated data. I don't really see the issue in here, if the favicon is switched because the user changed it in one crate, then so be it? |
QuietMisdreavus
commented
Apr 18, 2018
How about this: We can clone the |
GuillaumeGomez
commented
Apr 19, 2018
@QuietMisdreavus: Sounds like a good solution, I'll go for it then. |
GuillaumeGomez
commented
Apr 20, 2018
@QuietMisdreavus: Done! |
There was a problem hiding this comment.
As I've already said, the logo and favicon can also be different between crates. If you document two crates at the same time that have different logos, then which logo should the settings.html page use? I suggest setting the logo and favicon to an empty string here as well.
There was a problem hiding this comment.
This file should be generated each time to pick up changes to rustdoc options that can affect it.
There was a problem hiding this comment.
</div> is missing. Also the .to_owned() isn't needed.
bdeed13 to
ff5b7daCompareGuillaumeGomez
commented
Apr 21, 2018
Updated. Anything else you want me to change @ollie27 ? |
ollie27
left a comment
There was a problem hiding this comment.
Other than that one unneeded change this looks good to me.
There was a problem hiding this comment.
This change is no longer needed now that the logo has been removed.
ff5b7da to
1f7892fCompareGuillaumeGomez
commented
Apr 21, 2018
Done as well! |
GuillaumeGomez
commented
Apr 22, 2018
@bors: r=ollie27,QuietMisdreavus |
bors
commented
Apr 22, 2018
📌 Commit 1f7892f has been approved by |
bors
commented
Apr 22, 2018
…sdreavus Add rustdoc settings menu Fixes#18167. r? @QuietMisdreavus
bors
commented
Apr 22, 2018
☀️ Test successful - status-appveyor, status-travis |
Fixes#18167.
r? @QuietMisdreavus