-
-
Notifications
You must be signed in to change notification settings - Fork 809
fix: mobile toc issue with custom banners #3382
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ccd778d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Co-authored-by: Chris Swithinbank <[email protected]> Co-authored-by: HiDeoo <[email protected]> Co-authored-by: delucis <[email protected]>
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 for having a go at this @trueberryless!
this issue could also be resolved in some other ways of course and I'm not sure if introducing a second
IntersectionObserver
is a good practice just for this edge case. Feel free to suggest cleaner solutions!
I think you might be right here — we can probably aim to fix the underlying issue if we can.
I tried adding https://github.com/pomber/intersection-observer-debugger to the page which is a handy way to visualize what is happening when working with IntersectionObserver
and took a screenshot. This shows the problem quite clearly: the area we track using the rootMargin
(highlighted at the top in purple) and the page title (highlighted below in green) don’t intersect:

If we look at a page without the banner, we can see these areas intersect (yellow highlight), triggering the initial ToC state:

The correct fix is probably to see if we can make the getRootMargin()
method correctly detect an appropriate position by taking the banner into account:
starlight/packages/starlight/components/TableOfContents/starlight-toc.ts
Lines 100 to 110 in 266eed4
private getRootMargin(): `-${number}px 0% ${number}px` { | |
const navBarHeight = document.querySelector('header')?.getBoundingClientRect().height || 0; | |
// `<summary>` only exists in mobile ToC, so will fall back to 0 in large viewport component. | |
const mobileTocHeight = this.querySelector('summary')?.getBoundingClientRect().height || 0; | |
/** Start intersections at nav height + 2rem padding. */ | |
const top = navBarHeight + mobileTocHeight + 32; | |
/** End intersections `53px` later. This is slightly more than the maximum `margin-top` in Markdown content. */ | |
const bottom = top + 53; | |
const height = document.documentElement.clientHeight; | |
return `-${top}px 0% ${bottom - height}px`; | |
} |
Ah, although re-reading your second considered solution is related to that same idea. Hmm. I’ll have to think about that. |
Yeah exactly, if we include the height of the banner in the As I think that this problem would be much harder to solve with potentially more edge cases, I gave up on the idea quite early, but maybe there is potential 🤔 Also just to mention it if (maybe I forgot to say): It's impossible to change the
Notice the read-only. |
Yeah that’s correct, that’s why we destroy and recreate the observer on window resizes currently. |
Description
This PR fixes an issue with the Table of Contents being unable to find any heading when the page has a banner with custom styling. This issue is currently tracked in #3047.
The issue is fixed by introducing a temporary
IntersectionObserver
without anyrootMargin
(so that it definitely gets an intersection target). This new code only runs when there is no other intersecting target found by the existingIntersectionObserver
.Visit https://deploy-preview-3382--astro-starlight.netlify.app/getting-started/ and make the screen a smaller to see the mobile ToC, then reload and see that a current heading is set.
Please note that this issue could also be resolved in some other ways of course and I'm not sure if introducing a second
IntersectionObserver
is a good practice just for this edge case. Feel free to suggest cleaner solutions!Another considered solution
Another solution that came to my mind, but where I figured that it has too many side effects (and therefore I didn't commit to it): By dynamically incorporating the actual height of the banner in the calculation of
bottom
it would be possible to always guarantee that a heading can be found (which would mean that we don't need a second observer). However, the problem with that approach would be that the observer would then permanently be too big and headings would "too early" be considered current headings. For example: If the banner is really big, headings that scroll into the middle of the screen could potentially be set as the "current" headings in the ToC, which would be misleading.To demonstrate what I mean with this paragraph in code:
starlight/packages/starlight/components/TableOfContents/starlight-toc.ts
Line 107 in 2768932
Related stuff and todo