mirror of
https://github.com/snachodog/just-the-docs.git
synced 2025-04-10 14:01:22 -06:00
Fix incorrect positioning of clickable area for navigation links on Safari (#1403)
Closes #1392. Unfortunately, this PR has not actually diagnosed the root problem with the `scrollBy` calculation/method and Safari. However, by using the [`scrollIntoView`](https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView) function (which essentially does what the calculation was meant to do), this problem is "magically" solved! As a side effect, I think this makes the code easier to maintain (I myself was thinking: why is there a magic `3` multiplier?). ~~I will point out that this does change *how much is scrolled*; following the spec for the method, the sidebar is now scrolled so that the active navigation link is top-aligned with the scroll container (which in this case, is the navigation sidebar's "cutoff"). I personally am fine with this change, but happy to fiddle around (e.g. we could vertically align to the `center` via [`scrollIntoViewOptions`](https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView#parameters), though I'm not sure if this causes compatability problems).~~ I will point out that this does change *how much is scrolled*; we are now using the `center` option to `scrollBy`, which centers the target link. As Peter has commented in the PR thread, this seems to be the best compromise for maintaining the spirit of the previous calculation. ### testing Peter did a great job writing a reproducible bug report in #1392. To test this, 1. first, follow the instructions verbatim on `main`. observe that the bug appears on Safari on macOS, but not Firefox and Chrome 2. then, apply this change (e.g. check out the branch) 3. next, replay the instructions - observing that 1. the bug is fixed on Safari 2. there is no change to the behaviour on Firefox and Chrome (other than the "start" of the scroll") ### compatability On [Can I use](https://caniuse.com/scrollintoview), `scrollIntoView` has 98.15% adoption (the only "major" holdout being Opera Mini); and, the partial support from IE is about an option that we don't use. So, I'm pretty confident that we should be able to roll out this change without our users being locked out by a new-ish method.
This commit is contained in:
parent
0352428017
commit
da38718d7a
@ -17,7 +17,7 @@ This website is built from the `HEAD` of the `main` branch of the theme reposito
|
||||
|
||||
Code changes to `main` that are *not* in the latest release:
|
||||
|
||||
- N/A
|
||||
- Fixed: incorrect positioning of clickable area for navigation links on Safari by [@mattxwang] in [#1403]
|
||||
|
||||
Docs changes made since the latest release:
|
||||
|
||||
@ -30,6 +30,7 @@ Docs changes made since the latest release:
|
||||
[@mitchnemirov]: https://github.com/mitchnemirov
|
||||
|
||||
[#1390]: https://github.com/just-the-docs/just-the-docs/pull/1390
|
||||
[#1403]: https://github.com/just-the-docs/just-the-docs/pull/1403
|
||||
|
||||
## Release v0.7.0
|
||||
|
||||
|
@ -499,8 +499,7 @@ function navLink() {
|
||||
function scrollNav() {
|
||||
const targetLink = navLink();
|
||||
if (targetLink) {
|
||||
const rect = targetLink.getBoundingClientRect();
|
||||
document.getElementById('site-nav').scrollBy(0, rect.top - 3*rect.height);
|
||||
targetLink.scrollIntoView({ block: "center" });
|
||||
targetLink.removeAttribute('href');
|
||||
}
|
||||
}
|
||||
|
Loading…
x
Reference in New Issue
Block a user