-
Notifications
You must be signed in to change notification settings - Fork 13k
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
base: master
Are you sure you want to change the base?
Make impl section headers sticky #133717
Conversation
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
This comment has been minimized.
This comment has been minimized.
5b23dc6
to
cda1455
Compare
Forgot to push the second commit updating GUI tests... |
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. |
It's messed up when the sidebar is hidden. Otherwise, seems fine. |
cda1455
to
0591703
Compare
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. |
@rfcbot fcp merge |
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. |
Damn, only wanted front-end people. Sorry for the noise. @rfcbot fcp cancel |
@GuillaumeGomez proposal cancelled. |
@rfcbot fcp merge |
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. |
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:
|
(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) |
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