Skip to content
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

Fix rustdoc sidebar z-index #59973

Merged
merged 2 commits into from
Apr 16, 2019
Merged

Fix rustdoc sidebar z-index #59973

merged 2 commits into from
Apr 16, 2019

Conversation

Enity
Copy link
Contributor

@Enity Enity commented Apr 14, 2019

I think the screenshot will say everything:
image

live example: https://docs.rs/nom/4.2.3/nom/

I chose the smallest z-index to avoid problems with other blocks.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2019
@jonas-schievink jonas-schievink added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 14, 2019
@jonas-schievink
Copy link
Contributor

How do you get the sidebar overlap with the content? That seems like a bug.

@Enity
Copy link
Contributor Author

Enity commented Apr 14, 2019

How do you get the sidebar overlap with the content? That seems like a bug.

there are long blocks at the main content section (https://docs.rs/nom/4.2.3/nom/#macros).
We can customize the line break styles, but these are more breaking changes than in current mr.

By the way, the sidebar should be higher than the main content, right?

@jonas-schievink
Copy link
Contributor

Oh, okay, you have to scroll sideways with a small window.

The description for "Modules" is wrapping correctly, so there must still be another bug regarding the description of macros (and potentially other things). We still might want to merge the z-index change though.

@GuillaumeGomez
Copy link
Member

This isn't a good solution. We'd better set a max-width to the main container instead of playing with z-index.

@tesuji
Copy link
Contributor

tesuji commented Apr 15, 2019

Did you refresh the cached page (with Ctrl-F5 on Firefox)?

@Enity
Copy link
Contributor Author

Enity commented Apr 15, 2019

I researched a little more, and found the root of the problem.

image

Well, there is logic in it, and the wrapped code blocks look a little worse.
But we need to find a compromise.

image

@GuillaumeGomez
Copy link
Member

By adding a little spacing between two code blocks docs maybe?

@jonas-schievink
Copy link
Contributor

Yes, those descriptions should wrap like other descriptions, but the docs themselves could be improved by splitting them up. If the first paragraph is too long it always looks bad when rendered.

@Enity
Copy link
Contributor Author

Enity commented Apr 15, 2019

This isn't a good solution. We'd better set a max-width to the main container instead of playing with z-index.

If we set the "position: fixed" and do not specify the z-index, the render priority will be determined based on the order in the DOM. Why is an explicit sidebar positioning isn't a good solution?

Yes, those descriptions should wrap like other descriptions, but the docs themselves could be improved by splitting them up. If the first paragraph is too long it always looks bad when rendered.

Changes of word-wrapping should be in another pull request, yea?

@GuillaumeGomez
Copy link
Member

Changes of word-wrapping should be in another pull request, yea?

No, the same is fine.

@Enity
Copy link
Contributor Author

Enity commented Apr 15, 2019

its look fine now:

image

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Apr 15, 2019

📌 Commit 9e17193 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 16, 2019
…umeGomez

Fix rustdoc sidebar z-index

I think the screenshot will say everything:
![image](https://user-images.githubusercontent.com/2884517/56098429-37fa3680-5f09-11e9-8c54-4e2548aa0818.png)

live example: https://docs.rs/nom/4.2.3/nom/

I chose the smallest z-index to avoid problems with other blocks.
Centril added a commit to Centril/rust that referenced this pull request Apr 16, 2019
…umeGomez

Fix rustdoc sidebar z-index

I think the screenshot will say everything:
![image](https://user-images.githubusercontent.com/2884517/56098429-37fa3680-5f09-11e9-8c54-4e2548aa0818.png)

live example: https://docs.rs/nom/4.2.3/nom/

I chose the smallest z-index to avoid problems with other blocks.
bors added a commit that referenced this pull request Apr 16, 2019
Rollup of 6 pull requests

Successful merges:

 - #59717 (improve docs for std::hint::unreachable_unchecked())
 - #59903 (Continue evaluating after missing main)
 - #59973 (Fix rustdoc sidebar z-index)
 - #59992 (rustdoc: use --static-root-path for settings.js)
 - #59993 (include mode in unused binding suggestion span)
 - #60000 (Add repo-specific triagebot configuration)

Failed merges:

r? @ghost
@bors bors merged commit 9e17193 into rust-lang:master Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants