-
Notifications
You must be signed in to change notification settings - Fork 124
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
Update anchor navigation with scroll margin top #1953
Update anchor navigation with scroll margin top #1953
Conversation
@elroygohjy Sorry just a quick point: I think the last portion of the PR description is meant to be included, perhaps don't remove it next time? |
Oopss, accidentally removed it. I have added it back in |
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.
Thanks @elroygohjy! Just some initial comments.
Will have to implement this for the panels as well :)
Hi @elroygohjy, great work on this PR. Checking if you are still keen to implement the last few comments/fixes as requested? |
Hi @tlylt, @EltonGohJH would take a look into this. |
3a4a129
to
11c5cfc
Compare
@jonahtanjz |
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.
Thanks @EltonGohJH!
LGTM 👍
@EltonGohJH there's a conflicting file, could you help to resolve it? |
@EltonGohJH pinging, in case you missed it. |
42bc2b0
to
080bcd0
Compare
11c5cfc
to
b00a056
Compare
Hi @EltonGohJH, thank you for moving this PR further. I think everything works in general. However, I found a weird glitch: Going to the preview site and clicking on one of the heading works as expected. However, if I refresh this page multiple times, the heading first goes under the header, and then goes below the expected position. I tested it locally and the same thing occurs. The existing master does not have this behavior. Can you help to investigate? |
@tlylt |
I don't think refresh is any different from landing on a page with the anchor specified as part of the URL (maybe there are browser differences). For MarkBind, this issue might be relevant: #1150 , we do have a function that deals with scrolling. Perhaps can start from there. |
A interesting point would be that currently our refresh with anchor is buggy on firefox. (currently not my branch) But my current theory is that refresh does not trigger rescrolling which leads to the bug in my branch. MarkBind.-.User.Guide_.Formatting.Contents.Mozilla.Firefox.2023-03-20.17-01-30.mp4 |
@tlylt scrollToUrlAnchorHeading is not doing its job on reload as the I have replaced it to run only window.onload and removed I used this instead which will account scroll-margin-top when scrolling Overall this proves that scroll-margin-top is indeed the most correct approach as with it we can now remove the hacky fix. |
b00a056
to
16d0b87
Compare
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
What is the purpose of this pull request?
Overview of changes:
Resolves #1775
Unintentional but also resolves #1915, #1638
Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Checklist: ☑️