-
Notifications
You must be signed in to change notification settings - Fork 602
add a top-margin to all elements which get referenced by anchor-urls,… #340
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
Conversation
… so they are not covered by the fixed header
|
This change seems useful for browsers that support it and when JavaScript is off (or our JS is just... broken). The patch is tiny and to the best of my limited knowledge wouldn't hurt. The only changes that I would suggest are matching the margin that we add to the body (
We do have some JS to do "smooth scrolling", which can be pretty wonky at times. |
|
I was able to reproduce the scrolling issue reliably. it seems opening a url https://www.php.net/manual/en/opcache.configuration.php#ini.opcache.file_update_protection in a new browser tab makes some JS running which brings the text back into the users viewport (prevent it from beeing hidden behind the fixed header). but when you jump within the page, from e.g. the table at the top of https://www.php.net/manual/en/opcache.configuration.php by clicking one of the settings, the page scrolls down and the scroll-target gets hidden behind the sticky-header. I would propose disabling the autoscrolling javascript code (wasn't able to find it in the source-code -> pointers are welcome) and merging the CSS fix, to get a better overall experience. |
Agreed. For the JS... see js/common.js:26-97. |
|
thx for your feedback guys. I just went ahead, deleted the javascripts and added css-native smooth scrolling. |
salathe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
heiglandreas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 🇮🇹
|
Applied as 6c8c2aa. Thanks! 🎉 |
|
verified the fix in production. thank you guys! |
… so they are not covered by the fixed header
Repro
without the PRs patch, the anchor to which the browser scrolls to, is hidden behind the fixed header toolbar.
see this picture, in which I temporarily made the header transparent, so you can see what I mean:
with this fix applied, the browser knows that a margin of 60px should be used, so the actual content is visible to the end user.
After a bit more testing I get the feeling the site is scrolling via javascript to get the same feature across.
This seem to not work consistently, because sometimes I can get the browser to not scroll into the correct position. not sure how to reproduce this consistently right now tho.