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

Make impl section headers sticky #133717

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

Fixes #133506.

It makes impl sections headings sticky, allowing users to know what impl items they are currently going through.

You can test it here.

Some technical details: to make it work (because summary use both ::before and ::after pseudo-elements), I needed to give some left margin and padding so that the other collapse/expand toggles of children items would not appear on the sticky heading's toggle. Hence why the GUI tests new values look so weird.

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 1, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 1, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@aDotInTheVoid aDotInTheVoid added A-rustdoc-ui Area: Rustdoc UI (generated HTML) T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Dec 1, 2024
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Forgot to push the second commit updating GUI tests...

@joshtriplett
Copy link
Member

joshtriplett commented Dec 2, 2024

I love it! https://rustdoc.crud.net/imperio/sticky-headings/std/primitive.slice.html looks great.

I can confirm that it works as expected in Firefox on both desktop and mobile.

@notriddle
Copy link
Contributor

It's messed up when the sidebar is hidden. Otherwise, seems fine.

@GuillaumeGomez
Copy link
Member Author

Kinda fixed the mess up with the sidebar button. Not perfect but sadly, we can't set an element position based on the page scroll without JS, and with JS, having a listener on the page scroll is terrible for performance. So instead, I simply make the button above the heading. It'll prevent to collapse the heading if the button is present, but I think it's ok.

@GuillaumeGomez
Copy link
Member Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 11, 2024

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 11, 2024
@GuillaumeGomez
Copy link
Member Author

Damn, only wanted front-end people. Sorry for the noise.

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Dec 11, 2024

@GuillaumeGomez proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 11, 2024
@GuillaumeGomez GuillaumeGomez added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 11, 2024
@GuillaumeGomez
Copy link
Member Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 11, 2024

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 11, 2024
@notriddle
Copy link
Contributor

The trouble with this sort of thing is that it's really hard to get all the interaction features right. Here's a few cases that act weird:

  • If I click an item in the sidebar, it gets lost underneath the impl header.

    screen-record

    Normally, the solution to this one is scroll-margin-top, as in rustdoc mobile: fix scroll offset when jumping to internal id #93067, but, unfortunately, we don't know how tall the impl header is:

    screen-record

  • If I collapse an impl block while I'm scrolled some distance into it, I wind up at some arbitrary point in the document. I don't know how to fix this one without JS.

    screen-record

  • Hidden navigation bar is still a mess on mobile:

    screen-record

@notriddle
Copy link
Contributor

@rfcbot concern scroll-margin-top

@rfcbot concern scroll after collapse

@rfcbot concern mobile hidden persistent navigation bar

@Manishearth
Copy link
Member

(In favor of this general design, but echo notriddle's concern that this is really tricky to get right, so we should spend some time experimenting and ensuring it works in all scenarios)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: Please use sticky headers for impl blocks
8 participants