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

Fixes footer with new sidebar changes #1619

Merged
merged 1 commit into from
Feb 13, 2022

Conversation

GuillaumeGomez
Copy link
Member

Fixes #1613.

Since rust-lang/rust#92692, the sidebar width varies between 200 and 250 pixels, making it impossible to adapt the footer to this change without integrating it into the rustdoc main container directly. Another interesting change that was added to the sidebar was position: sticky which allowed me instead to move the footer completely at the bottom and to add margin-bottom on the sidebar. Here is a video of the change when you scroll to the end of the page:

Peek.2022-01-22.21-30.mp4

cc @syphar @jsha

@jsha
Copy link
Contributor

jsha commented Jan 22, 2022 via email

@jsha
Copy link
Contributor

jsha commented Jan 22, 2022 via email

@GuillaumeGomez
Copy link
Member Author

I'm not sure about this. Needs to ask others what they think about it. A big problem is that the top navbar is already very "heavy" content-wise.

@GuillaumeGomez
Copy link
Member Author

Also I'm not super attached to the rustdoc sidebar varying width. It seemed maybe nice to give back some more space on smaller screens and narrower docs, but if it causes other design headaches we can just ditch it.

The problem is that some crates already use this new style. And also, I personally like it (but it's just my opinion).

@syphar syphar added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jan 23, 2022
@syphar
Copy link
Member

syphar commented Jan 30, 2022

r? @Nemo157

@Nemo157
Copy link
Member

Nemo157 commented Jan 30, 2022

Empirically it appears that this style works on docs from 2021-12-14 just fine if .sidebar-menu also allows .sidebar-elems. It'd be good to reduce the variations if possibe.

@GuillaumeGomez
Copy link
Member Author

Empirically it appears that this style works on docs from 2021-12-14 just fine

You mean 2021-12-05?

if .sidebar-menu also allows .sidebar-elems. It'd be good to reduce the variations if possibe.

You mean having this rule instead: .sidebar-elems, .sidebar-menu { ... }?

@Nemo157
Copy link
Member

Nemo157 commented Jan 30, 2022

You mean 2021-12-05?

Well, 2021-12-14 is one of the versions that would use the 2021-12-05 css and happened to be the one I found first while looking for a crate to test on.

You mean having this rule instead: .sidebar-elems, .sidebar-menu { ... }?

Yep. In fact, .sidebar-menu doesn't appear to exist at all in the rustdoc html?

@GuillaumeGomez
Copy link
Member Author

It existed in 1.58: https://doc.rust-lang.org/1.58.0/std/

@Nemo157
Copy link
Member

Nemo157 commented Jan 31, 2022

Yep, but not in 2022-01-19+ that this css is targeting (or I think in 2021-12-05+ that the previous css was targeting).

@GuillaumeGomez
Copy link
Member Author

I updated the date to 2021-12-14.

@syphar
Copy link
Member

syphar commented Feb 6, 2022

@GuillaumeGomez @Nemo157 how can we proceed here?

I see a test is failing, what else is missing?

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Feb 6, 2022
@Nemo157
Copy link
Member

Nemo157 commented Feb 6, 2022

The reason I brought up this potentially working with all post-2021-12-05 docs is that we really need to minimize the number of css variations we have. The more we need to maintain the harder it becomes to do any changes that affect them (especially as we don't have a really easy way to test older versions outside deploying to production and checking the prebuilt docs).

My assumption is that if it is a change from 2022-01-19 that prompts this change, and 2021-12-14 works, then everything from 2021-12-05 -- 2022-01-19 will probably work too, and we can just change the 2021-12-05 css and not introduce a new variation.

@GuillaumeGomez
Copy link
Member Author

Finally took the time to come back to this. @Nemo157 was right: it works just fine with 2021-12-05 crates as well.

@syphar
Copy link
Member

syphar commented Feb 13, 2022

@GuillaumeGomez thanks for checking!

so this is ready for another review? perhaps by @Nemo157 ?

@Nemo157 Nemo157 merged commit ee84c34 into rust-lang:master Feb 13, 2022
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Feb 13, 2022
@GuillaumeGomez GuillaumeGomez deleted the fixes-sidebar branch February 13, 2022 12:05
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Footer is broken with new sidebar changes
4 participants