Skip to content

Fix new sidebar changes #1579

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 9, 2022
Merged

Fix new sidebar changes #1579

merged 1 commit into from
Jan 9, 2022

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Dec 13, 2021 •
edited
Loading

Fixes #1574.

Screenshot from 2021-12-13 22-13-04
Screenshot from 2021-12-13 22-21-56

This PR fixes a few issues following the sidebar layout changes.

@Nemo157 Nemo157 linked an issue Dec 13, 2021 that may be closed by this pull request
Recent docs build renders the left column oddly #1574
Closed
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 14, 2021
Closed
@GuillaumeGomez
Copy link
Member Author

Now that I think about it, I don't think docs.rs css files have versions. Do we have a way to make it work starting a given rustdoc version?

@Nemo157
Copy link
Member

Nemo157 commented Dec 14, 2021

Yeah, was just gonna say this causes issues on existing pages:

image

AFAIK there is no current setup to do per-version css.

@Nemo157
Copy link
Member

Nemo157 commented Dec 14, 2021

There's also an issue that the first scroll of the body causes the sidebar to also scroll up on new docs.

@GuillaumeGomez
Copy link
Member Author

Indeed, good catch. I'll send a fix for the scroll bug. Any idea for the css versioning?

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 14, 2021 •
edited
Loading

We could take advantage of rustdoc options to solve this issue. For example using --html-before-content to create an element with a given ID. Or we could put it into a new CSS file and link it with --html-in-header. What do you think?

@GuillaumeGomez GuillaumeGomez force-pushed the new-sidebar branch 2 times, most recently from dbff700 to 4aa248e Compare December 14, 2021 23:13
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 14, 2021 •
edited
Loading

Something kinda like this would work. Having a CSS file based on the rustdoc version would be even better (and it can be included into the header instead of the inlined CSS I put as a POC!).

@GuillaumeGomez GuillaumeGomez force-pushed the new-sidebar branch 3 times, most recently from 0d936f0 to 82c5404 Compare December 16, 2021 14:52
@GuillaumeGomez
Copy link
Member Author

I finally did as follows: we generate a new <link> into the newly generated crates' documentation <head> so that it uses a new CSS overloading the old one. So the old crates style is still working and the new ones don't have issues anymore.

@notriddle
Copy link
Contributor

notriddle commented Dec 23, 2021 •
edited
Loading

This does not seem like a sustainable, long-term solution.

It looks like the doc_rustc_version is stored in the releases table. Could that be used to pick between two different CSS files?

@GuillaumeGomez
Copy link
Member Author

I'll take a look. If the date is included in the version, then we will be able to completely go around this problem!

@GuillaumeGomez GuillaumeGomez force-pushed the new-sidebar branch 4 times, most recently from e32ddde to 60c8bd9 Compare December 24, 2021 19:18
@GuillaumeGomez
Copy link
Member Author

@notriddle Thanks a lot for your idea! It allowed me to determine when receiving the query for rustdoc.css which one I should provide depending on the rustdoc version used to generate it. Like that, the problem is fixed as soon as this fix is merged!

@Nemo157
Copy link
Member

Nemo157 commented Dec 24, 2021

Using the recorded date rather than changing the generated docs is definitely much nicer.

I think rather than sending the date as part of the request query I think we should just embed different /-/static/... urls in the html; that will cache slightly better as it only has the 2 different urls for the different css versions, instead of one url per-nightly.

@GuillaumeGomez
Copy link
Member Author

You mean generating the correct CSS path depending on the rustdoc version in the tera template?

@Nemo157
Copy link
Member

Nemo157 commented Dec 24, 2021

Yep.

@GuillaumeGomez
Copy link
Member Author

Makes sense! I'll update to do so then.

@GuillaumeGomez GuillaumeGomez force-pushed the new-sidebar branch 2 times, most recently from 384ebf5 to cd49cb1 Compare December 25, 2021 11:10
@GuillaumeGomez
Copy link
Member Author

Updated! Thanks a lot for the suggestion @Nemo157!

jyn514
jyn514 reviewed Dec 28, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Can you add a test or two for the new file? All the stringly-typed "rustdoc-2"s make me nervous.

@GuillaumeGomez
Copy link
Member Author

Can you add a test or two for the new file?

Good idea! I will do soon.

@GuillaumeGomez
Copy link
Member Author

Actually I'm wondering: what tests do you have in mind?

@GuillaumeGomez GuillaumeGomez force-pushed the new-sidebar branch 3 times, most recently from 9dd2edf to 747a28c Compare December 29, 2021 12:01
@GuillaumeGomez
Copy link
Member Author

I added a test @jyn514. Hope it's the one you had in mind.

jyn514
jyn514 approved these changes Dec 29, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Yup, this is great, thanks!

@syphar
Copy link
Member

syphar commented Jan 8, 2022

I did a manual test, I see that new doc-builds are definitely fixed, and the additional CSS file is added only in docs built with newer rustc version. So that's fine with me.

@Nemo157 @GuillaumeGomez did one of you do a manual test of the last version with old docs?

I recently wiped my local db & s3, so I don't have any. If you don't have them either, I would fetch some old archives from S3.

If that's tested I'll merge and deploy.

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

I painfully did. :')

@syphar
Copy link
Member

syphar commented Jan 9, 2022

I painfully did. :')

Reading my question from yesterday evening again my wording was a little off.

I mean a manual test with the latest state in this PR, testing a release / build that was done before december 6th.

@GuillaumeGomez
Copy link
Member Author

I understood it this way and I confirm that's what I did. ;)

@syphar syphar merged commit 17de230 into rust-lang:master Jan 9, 2022
@syphar syphar 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-review Status: This pull request has been implemented and needs to be reviewed S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it labels Jan 9, 2022
@GuillaumeGomez GuillaumeGomez deleted the new-sidebar branch January 9, 2022 17:00
@syphar syphar mentioned this pull request Jan 16, 2022
Closed
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 22, 2022
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@syphar syphar syphar approved these changes

@jyn514 jyn514 jyn514 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Recent docs build renders the left column oddly
5 participants
@GuillaumeGomez @Nemo157 @notriddle @syphar @jyn514