- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
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 ? | 
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