-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Add rustdoc settings menu #49954
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 rustdoc settings menu #49954
Conversation
|
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
0d222ac
Compare
|
Ready it is! |
|
If <a href='./std_unicode/index.html'><img src='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. |
|
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 |
|
☔ The latest upstream changes (presumably #50033) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Another option would be to use a pop-up like the help pop-up. |
|
@ollie27: Check out the changes, I remove the url on the logo. :) |
0d222ac to
f2ad3c3
Compare
|
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. |
|
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? |
|
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? |
|
How about this: We can clone the |
|
@QuietMisdreavus: Sounds like a good solution, I'll go for it then. |
|
@QuietMisdreavus: Done! |
src/librustdoc/html/render.rs
Outdated
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.
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.
src/librustdoc/html/render.rs
Outdated
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 file should be generated each time to pick up changes to rustdoc options that can affect it.
src/librustdoc/html/render.rs
Outdated
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.
</div> is missing. Also the .to_owned() isn't needed.
bdeed13 to
ff5b7da
Compare
|
Updated. Anything else you want me to change @ollie27 ? |
ollie27
left a comment
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.
Other than that one unneeded change this looks good to me.
src/librustdoc/html/layout.rs
Outdated
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 change is no longer needed now that the logo has been removed.
ff5b7da to
1f7892f
Compare
|
Done as well! |
|
@bors: r=ollie27,QuietMisdreavus |
|
📌 Commit 1f7892f has been approved by |
…sdreavus Add rustdoc settings menu Fixes #18167. r? @QuietMisdreavus
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #18167.
r? @QuietMisdreavus