feat(www): useActiveHash hook for highlighting links in Docs' table of contents

[jlkiri]

Description

Introduces a useActiveHash hook which can be used to detect which section of a page one is currently looking at and update URL hash accordingly. Now Table of Contents in Docs highlights links that are currently active (= link's hash is equal to URL hash).

Detection is done with IntersectionObserver. Not quite sure about which portion of viewport to use to detect intersections. Also I think that refactoring Tutorial to use this hook should be done in other PR.

Here is a demo GIF:

Related Issues

Addresses #21640

[gillkyle]

This is looking great! I'm seeing one issue in particular that keeps me from saying we should merge this, though I think it's going to be a great improvement 🙂

[gillkyle]

I'm also interested in the performance aspect of this. Locally it takes a moment for the ToC to update and I guess that won't be there in the actual build, but I fear that if it does, the JavaScript will block the main thread and keep you from being able to click on links (at least that's what I'm seeing when I run this on my machine).

I like the potential gains from these changes but I want to make sure they don't cause problems too!

[tesseralis]

One thing to consider would be the behavior in the mobile view:

We might want to disable this behavior on mobile, since it's kind of silly to update the current location for a component that you can't even see from that location. I do think that there can be improvements in how the table of contents is displayed on mobile (e.g. a menu you can click to expand it) but that's a story for another issue and another PR.

[tesseralis]

Made the issue for mobile TOC: #21807.

[tesseralis]

Just some minor nits left, otherwise looks good to me.

[muescha]

Just a note:
Did we replicate here something and have functionality implement twice (I dont't looked into code.so far)?

But:
While reading https://www.gatsbyjs.org/tutorial/part-zero/ i see also in the left sidebar the subtopics of the current section are also activated

[tesseralis]

@muesch The intention is to change the implementation of the tutorial sidebar to use the useActiveHash hook, but we'll leave that for another PR.

[gillkyle]

I pulled it locally and it looks good to me, I'll leave it up to @tesseralis's last feedback. Some notes (that aren't blocking) are:

• the intersection observer is looking for the heading (so scrolling up into the content of a longer section like "Heading A" will keep "Heading B" highlighted until the text for the heading is scrolled to)
• looks like only H2's are highlighted, if you are on another page like /docs/creating-a-source-plugin/ subheaders won't highlighted (which is maybe intentional 🤷‍♂)

Thanks for the work on this @jlkiri!

[jlkiri]

@gillkyle Yes, the reason is that text is not split in sections with their own ids, so there is not anything to tell Intersection Observer to watch ☹️ . As for subheaders - this is intentional at least for now, because only minimal changes are enough. Highlighting only a subheader (without its parent) is also suboptimal - ideally both parent header and subheader are highlighted. That requires more code but in other PR maybe.

[gillkyle]

Hey again @jlkiri! Talked over this with @tesseralis a little more. I think it's really close. To break down the last little pieces:

Intersection Observer on content instead of headings - this does seem like a tricky thing to get just right, and it's only a noticeable problem on really big sections of content when you're scrolling vertically (which isn't how most things, if any, are read anyway). So I think that behavior is fine for now ✅ (though the Apollo docs which are a Gatsby site have this implemented)

Subheader highlights - I don't think it's suboptimal to have just the child header highlighted, Docusaurus does it that way and it seems okay (https://v2.docusaurus.io/docs/introduction) would that be a hard adjustment? Seems like it might be updating getHeadingIds()?

Testing - Probably should have mentioned this sooner but is there something we can add to make sure we don't get regressions with this later as code around it evolves? With the pages that broke as it was being tested I hope it won't encounter other edge cases in useActiveHash hook that break things.

[jlkiri]

@gillkyle

Subheader highlights - OK. I made it a default in 5f051fd. Now subheaders are also highlighted.

Testing - Tbh I don't know how this can be tested meaningfully. I think we could have a unit test for getHeadingIds that will fail if current MDX schema changes. However I do not know how to write a test that checks an actual GraphQL response. And other components that use GraphQL are just tested with mocks anyway. Apart from that, by looking at current schema I do not see how it might fail elsewhere.

Cloud Tests seem to be failing but I do not know why...

[gillkyle]

Sorry it's been hard for me to keep getting back to you on this @jlkiri. Things are busy and we've got a lot we're trying to do all at once! I talked to @marcysutton about testing and these are some things she suggested:

• testing with stub data that doesn't change so things aren't brittle (in reference to the MDX schema changing)
• using data-testid attributes or similar for UI tests so you're not relying on IDs or other selectors which can change or mutate

Here are a couple places in code that could help serve as a reference.
Component code: https://github.com/marcysutton/gatsby-a11y-workshop/blob/master/examples/dropdown/dropdown.js#L50
Test: https://github.com/marcysutton/gatsby-a11y-workshop/blob/master/examples/unit-testing/dropdown.test.js

You can ignore the Cloud tests, this code won't touch that and we can sort those out internally. 🙂

[jlkiri]

@gillkyle I added tests in 63373b8! Tried to use both stub data and data-testids.

[gillkyle]

Thanks @jlkiri! I noticed one last issue that I updated in c2ed59a, some pages that have tableOfContentsDepth defined won't have subheaders shown in the ToC so after you scrolled to a subheader nothing would be underlined. I set the getHeadingIds function to only provide ids for the depth that's passed in which seems to be working. I'm inclined to feel like we should merge this once tests pass as it's working great on my end and seems to build without anymore problems on any other pages too 🙂

[gillkyle]

Thanks for all the hard work on this! (turned out having a lot more to it than we thought 😅) but it's a nice addition to the docs 🎉