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.

Sign up for GitHub

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

Jump to bottom

Improve rustdoc layout #91356

Merged
merged 13 commits into from
Dec 5, 2021
Merged

Improve rustdoc layout #91356

merged 13 commits into from
Dec 5, 2021

Conversation

GuillaumeGomez
Copy link
Member

This is an overtake of #89385 originally written by @cynecx.

I kept the original commit and simply added the missing fixes into a new one. You can test it online here.

r? @jsha

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Nov 29, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2021
@GuillaumeGomez GuillaumeGomez mentioned this pull request Nov 29, 2021
Closed
@GuillaumeGomez
Copy link
Member Author

I also renamed the main HTML ID into main-content to avoid confusion with the new main element. However it's a big change in term of line, so if preferred, I'll move it into another PR.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

I fixed the issue and updated the online docs.

jsha
jsha requested changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! A few small nits and some naming discussion:

I find the hierarchy main > .main-inner > #main-content confusing; I would have expected main-inner to be the innermost. The primary function of .main-inner seems to be a place to hang max-width: 960px (for doc pages) or max-width: unset (for source pages). Maybe we can call it more precisely .width-limiter or something?

src/librustdoc/html/static/css/rustdoc.css
@@ -248,6 +247,32 @@ textarea {

/* end tweaks for normalize.css 8 */

.rustdoc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our <body> tag always has the .rustdoc class, and the .rustdoc class is (AFAICT) only on the <body> tag. I think it would be clearer to attach these styles to the existing body rules.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we can't because of docs.rs. :-/

src/librustdoc/html/static/css/rustdoc.css Outdated
overflow-y: auto;
width: 300px !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure !important is not needed here and should be removed (https://csswizardry.com/2012/11/code-smells-in-css/).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked and you're right, so removed.

src/librustdoc/html/static/css/rustdoc.css
padding-top: 0px;
}

.rustdoc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above this rule should go on body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer as above. :-/

src/librustdoc/html/static/css/rustdoc.css
@@ -1678,7 +1768,7 @@ details.rustdoc-toggle[open] > summary.hideme::after {
padding: 0;
}

.sidebar .logo-container {
.rustdoc:not(.source) .sidebar .logo-container {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing a bunch of not() selectors usually hints that we're missing some positive class that would be useful. For instance it looks like it would be useful to add a doc class to the body tag whenever we're not in source view. But I don't feel that's required as part of this PR.

@GuillaumeGomez
Copy link
Member Author

Looking great! A few small nits and some naming discussion:

I find the hierarchy main > .main-inner > #main-content confusing; I would have expected main-inner to be the innermost. The primary function of .main-inner seems to be a place to hang max-width: 960px (for doc pages) or max-width: unset (for source pages). Maybe we can call it more precisely .width-limiter or something?

Agreed, I added a commit for the renaming.

I also updated the files on the server so you can check too. :)

@jsha
Copy link
Contributor

jsha commented Dec 5, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 5, 2021

📌 Commit d7528e2 has been approved by jsha

@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 Dec 5, 2021
@bors
Copy link
Contributor

bors commented Dec 5, 2021

⌛ Testing commit d7528e2 with merge e2116ac...

@bors
Copy link
Contributor

bors commented Dec 5, 2021

☀️ Test successful - checks-actions
Approved by: jsha
Pushing e2116ac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 5, 2021
@bors bors merged commit e2116ac into rust-lang:master Dec 5, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 5, 2021
@GuillaumeGomez GuillaumeGomez deleted the improve-rustdoc-layout branch December 5, 2021 21:41
@bors bors mentioned this pull request Dec 5, 2021
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e2116ac): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

  • Large regression in instruction counts (up to 1.7% on full builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 6, 2021
@GuillaumeGomez
Copy link
Member Author

There is basically no change in the rust source code so this is spurious.

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Dec 6, 2021
@cynecx cynecx mentioned this pull request Dec 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cynecx cynecx cynecx left review comments

@jsha jsha jsha requested changes

Assignees

@jsha jsha

Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
8 participants
@GuillaumeGomez @rust-highfive @rust-log-analyzer @jsha @bors @rust-timer @cynecx @rustbot