Skip to content
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

feat: Add sticky table of contents #178

Merged
merged 37 commits into from
Jan 8, 2025

Conversation

gingerchew
Copy link
Contributor

@gingerchew gingerchew commented Dec 24, 2024

Adds new <TableOfContents/> component, a sticky table of contents to be used in ProseLayout.

Takes one prop, headings, which is an array of MarkdownHeadings.

If there is less than 2 headings, the table of contents is not rendered.

Inside the<TableOfContents /> is a custom element <table-of-contents> that handles all the intersection observer logic and activating/deactivating Annotations.

It seems a shame that the way to get this to work is to have there
be a heading `## Contents` or `## table of contents` or `## toc`

am I overcomplicating something that is simple enough as is? Yes :)
@gingerchew
Copy link
Contributor Author

image

I can go in and add some styling but this is just the default remark plugin output.

It requires that there be a heading in the content ## Content or something similar that matches an regex. Seems like a thing that is easy to forget to do to me but it is what it is I suppose.

@evadecker
Copy link
Member

Nice! There should be a way to show the TOC without requiring ##Content. I'd take a look at https://github.com/beerose/aleksandra.codes for an example (see https://www.aleksandra.codes/astro-supabase for what it looks like in-use).

The Starlight repo may also be a good place to look for inspiration: https://github.com/withastro/starlight

@gingerchew
Copy link
Contributor Author

Okay, that was what I had in mind originally before you brought up the remark plugin. Looks like the examples show it can be done with just the headings that render generates.

I've added that, going to work on positioning it now, did you want the scrolling effect as well?

@gingerchew
Copy link
Contributor Author

image Here's with a smidge of grid and some position sticky image Here's after scrolling a bit, I just used `--space-2xl` for the top, but that can be bike-shed later (obvi)

@evadecker
Copy link
Member

Ooh nice, don't even need a plugin! That's great!

Would be nice to:

  • Remove default underlines
  • Some style to indicate the current section: maybe we can play around with the annotation library to add an animated underline?

This lets the table of contents move more dynamically
by utilizing `grid-area`
we can place things a little more sanely

for simplicity's sake, I used Bootstraps mobile breakpoint (576px)

Also some small font changes to make the toc look similar to the example provided
This will draw an underline as the headings come into view
then remove it as they scroll off screen.

It looks like RoughAnnotation doesn't have a way to animate the hiding though since the current
strategy is just to nuke the path elements it creates.
@gingerchew
Copy link
Contributor Author

Okay, so good news, bad news.

Good news: The animation is in and it looks really slick
Bad news: If we want it to be an indicator of what is currently in view and the underline should disappear after the element scrolls off screen, then the animation gets a little wonky

This is a known thing with RoughAnnotation, the way they "hide" the annotation is to just nuke the elements inside the svg. Here's an issue going back three years where they mention looking into it.

@gingerchew
Copy link
Contributor Author

I tried to grab a screen recording of the animation, but the gist is that it animates the first time, then after showing again after being hidden, the animation doesn't play.

@gingerchew gingerchew changed the title Adds remark-toc remark plugin Adds TableOfContents to ProseLayout Dec 27, 2024
Copy link
Member

@evadecker evadecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me awhile to get back to this review! Hope you've had a nice holiday and new years :)

Overall, I'm really excited with the progress here! With a few tweaks this should be good to go:

  1. We can disable animations for now to keep it simple.
  2. We need to design for mobile/tablet width and reposition the TOC
  3. We can define the annotations specific to TOC—they don't need to inherit from the existing config.
  4. Make sure the TOC works on pages like /terms and /privacy.

src/components/TableOfContents.astro Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/layouts/ProseLayout.astro Show resolved Hide resolved
src/layouts/ProseLayout.astro Outdated Show resolved Hide resolved
src/pages/[id].astro Outdated Show resolved Hide resolved
src/pages/blog/[id].astro Show resolved Hide resolved
src/pages/[id].astro Show resolved Hide resolved
astro.config.mjs Outdated Show resolved Hide resolved
removes data-annotation configuration
Since intersection observer calls on any *intersection* there was an issue where the annotation for a heading *leaving* the viewport would supercede the heading that had entered previously.

This addition will check against the scroll direction if the new heading is exiting or entering.
@gingerchew
Copy link
Contributor Author

Alright, created a PR for the content layer loader. I was hoping I could just feed the data store markdown and get the headings for free, but had to generate that metadata myself -.- Still a very nifty API and really fun to work with.

@evadecker evadecker changed the title Adds TableOfContents to ProseLayout feat: Add sticky table of contents Jan 6, 2025
@evadecker
Copy link
Member

This is looking good! A few things:

  1. Can we reduce the size of "On this page" to the body size? font—size: var(--step-0); You may need to tweak the line height as well.

  2. Can we add scroll-margin-top to headings to prevent them from being flush with the edge of the browser when the section is jumped to?

CleanShot 2025-01-06 at 17 20 05@2x

  1. The logic for displaying the active section is better, but still could be improved: e.g. in the below video, it skips sections that are too small for the header to intersect the browser edge.
CleanShot.2025-01-06.at.17.02.48.mp4

Here's an idea for logic we could use (but if you have a better idea, let me know):

CleanShot 2025-01-06 at 17 24 32@2x

@gingerchew
Copy link
Contributor Author

Okay, 1 and 2 have been resolved.

But as for # 3... I'm pretty sure I just solved it. I'm going to clean it up and comment it so that someone coming back to it can understand it. I had to orchestrate 2 separate intersection observers to track if an element was entering the bottom half or the top half of the screen.

It took two intersection observers to do it, but I got it working.

Using the boundingClientRect property of the IntersectionObserverEntry
we can determine whether the element is scrolling into or out of the top half

With this, the top io will run the annotation when the element enters the top half
then the bottom io will run the previous annotation when a heading enters the
bottom half of the screen
@gingerchew
Copy link
Contributor Author

I tested it against the same page as in the video and it looks like it is working flawlessly now :)

Copy link
Member

@evadecker evadecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shaping up great! I like the web component approach, it's clean!

Left a few comments on areas I think we can clean up the JS and CSS.

src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/components/TableOfContents.astro Outdated Show resolved Hide resolved
src/layouts/ProseLayout.astro Outdated Show resolved Hide resolved
src/layouts/ProseLayout.astro Show resolved Hide resolved
@gingerchew
Copy link
Contributor Author

Okay, I feel like I understand and do not understand at the same time, so I went to tldraw to draft it out visually, and I think this makes sense? Tell me if I'm in the right direction:

https://www.tldraw.com/ro/9bGuunSsgiN7Nw9DRHMCN?d=v178.-655.2365.1693.page

I feel like this should be easier and I've got a mental block somewhere that is making it seem super difficult and complex.

@evadecker
Copy link
Member

evadecker commented Jan 8, 2025

Okay, I feel like I understand and do not understand at the same time, so I went to tldraw to draft it out visually, and I think this makes sense? Tell me if I'm in the right direction:

https://www.tldraw.com/ro/9bGuunSsgiN7Nw9DRHMCN?d=v178.-655.2365.1693.page

I feel like this should be easier and I've got a mental block somewhere that is making it seem super difficult and complex.

No, it's okay! This is surprisingly tricky to reason about. The UX is so simple and straightforward, so it feels like the code should be, too, but the IntersectionObserver API is kind of confusing.

CleanShot 2025-01-07 at 22 40 30@2x

Your logic looks solid to me. But don't spend too much longer here—I'm super pleased with the functionality as it exists! Just curious if there's a way to simplify the code any more.

Much cleaner, less observers, better logic on when and what to trigger
@gingerchew
Copy link
Contributor Author

Unfortunately you triggered my ape brain and I had to solve it, thankfully a good nights sleep unlocked the right neurons to solve it :)

@evadecker
Copy link
Member

This is looking and working beautifully! Scrolling works as intended, fluid scrolling up and down the page.

I only encountered one issue:

When clicking links in the TOC, it works correctly if you click a link below the current active link. However, if you click a link above the active link, it highlights the item above it, even though it isn't in view.

CleanShot.2025-01-08.at.10.02.27.mp4

Changes the activeIndex initial value to -1 to mark it as uninitialized

extracts the annotation updating into its own function

moves observation into a separate callback to prevent accidental activation on load
@gingerchew
Copy link
Contributor Author

Okay, this time I think I got it. I did some extra cleanup extracting the annotation animation to its own function.

Also had to move the observing to it's own reversed array loop. There was an issue where on load the second to last item in the TOC would be annotated, by starting from the end of the list and moving to the top, none of them are annotated.

Copy link
Member

@evadecker evadecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works! Fantastic! Thanks so much for all your effort here. I may have to steal some of this logic for my personal site. 😈

this.#headings.toReversed().forEach((h) => this.#io.observe(h));
}

updateAnnotation() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧑‍🍳👌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
love to see well abstracted code

@evadecker
Copy link
Member

Once we add a short description as PR comment to the top (for future reference) I can merge this in! 💟

@gingerchew
Copy link
Contributor Author

Comment added :)

@evadecker evadecker linked an issue Jan 8, 2025 that may be closed by this pull request
@evadecker evadecker merged commit d34b440 into namesakefyi:main Jan 8, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support displaying automated table of contents on ProseLayout
2 participants