152 rhs toc

[olayway]

Closes #152

Changes

• ToC on the right-hand side of the page
  • items indented according to their level
  • fixed positioning
  • smooth-scroll to a given section upon clicking on its title in the ToC
  • current heading highlighted in yellow
  • hidden for breakpoints < lg

Also:

• headings in the content can be clicked, which results in scrolling to their start

Preview

[netlify]

✅ Deploy Preview for boring-fermat-22cb64 ready!

Name	Link
🔨 Latest commit	ba190c7
🔍 Latest deploy log	https://app.netlify.com/sites/boring-fermat-22cb64/deploys/629f30ed7c623a0008a2e669
😎 Deploy Preview	https://deploy-preview-167--boring-fermat-22cb64.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[khalilcodes]

hey @olayway please see following comments for this PR.

• some observations:

  • a left and right padding has been added to the homepage layout
  • there is a Sidebar.js component not being used
  • a package.json outside the site folder

• suggestions:

  • hooks like getObserver would be best in maybe lib or a hooks folder

I feel there are a few moving parts to the code and was thinking maybe it's best if we keep everything in a single Toc file with the getObserver stuff so that it can also be reused as a drop in component since we have both left and right. eg. <TocComponent right .... />

Overall I like your approach 👍, I can work further on this if you dont mind.

[olayway]

@khalilcodes, thank you for your review!

I'd like to continue working on this feature 😉 Let me check your suggestions and update the code accordingly. I'll take care of it probably tomorrow.

As for these two comments of yours:

there is a Sidebar.js component not being used

Right... I have created a basic left-hand side component at the beginning to test how the layout will work when we have both LHS and RHS ToCs. But it shouldn't go into this PR!

a package.json outside the site folder

Again, my bad. Thanks for spotting that :)

[olayway]

Hi @khalilcodes! Once again, thank you for your review 👏 🎊

Here is the full response to your comments.

• a left and right padding has been added to the homepage layout

Hmm... I haven't touched the index.js file. Paddings have only been added to site/components/layout.js which is used by all other pages (apart from /all and /concepts). But maybe I've misunderstood you?
EDIT: Actually you're right. I'll fix that now!
EDIT2: Done.

• there is a Sidebar.js component not being used

I've removed it. Thank you for your vigilance! 👏

• a package.json outside the site folder

Removed. Thank you! 👏

• hooks like getObserver would be best in maybe lib or a hooks folder
  I feel there are a few moving parts to the code and was thinking maybe it's best if we keep everything in a single Toc file with the getObserver stuff so that it can also be reused as a drop in component since we have both left and right. eg. <TocComponent right .... />

I totally agree. There was too much going on in the MDX component. I've moved the logic related to keeping track of the current active heading to the custom hook getHeadingsObserver and placed it in the new site/hooks folder.

As for creating a separate component for this: I tried this approach in the first go when I was trying to extract headings from the markdown and create a ToC component myself. However, since we are using a rehype plugin rehype-slug for automatically adding ids to headings, I didn't want to run the risk of creating ids for the ToC component that would not match the ones created by the plugin... If I were to do that, I would have basically had to repeat part of the code run by the rehype-slug plugin (re crating ids) or somehow parse the resulting HTML to get the headings with ready ids (added by rehype). I didn't want to do that + I've found a rehype plugin rehype-toc that generates the ToC. It makes use of ids added first by the rehype-slug and generates the ToC (wrapped in <nav></nav> element). One drawback of using this plugin is that it can only attach this ToC to the root element of the page content's hast -> i.e. it will end up somewhere in the <main> tag inside of the <article> (depending on the options passed to the plugin). However, I don't think this is too big of an issue since:

1. The ToC logically belongs to the article's content
2. The plugin also attaches classes (specified in the docs) to each of the ToC element. Thus, not only each element within the ToC can be customized but also the ToC's parent element can be made fixed, which enables positioning it wherever on the screen we want (and this is actually a desired behavior to have it always visible).

You're right about that this approach prevents us from having a re-usable ToC component, but I don't think it is necessary. I don't think we should show site contents both on the RHS and in the LHS sidebar at the same time. I think on the LHS sidebar we should display different pages of the site, but not necessarily contents of each. However there is a situation when we could have the ToC on the LHS -> for smaller screens (atm the ToC only visible for large screens). In this case, we could move it above the content of the Sidebar. Here is the example of this behavior: https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API. I think it is pretty nice and it would only require us to work on CSS a bit. I'd love to hear your opinion on that!

Additional changes:
I've also figured out the real reason why no heading was highlighted on the initial page render (before scrolling) so I've simplified the code even more.

[khalilcodes]

@olayway really thanking you for this detailed explanation. 🙏

I've moved the logic related to keeping track of the current active heading to the custom hook getHeadingsObserver and placed it in the new site/hooks folder.

Yes agree it looks much better now 👍

I've found a rehype plugin rehype-toc that generates the ToC. ...

I don't see any drawbacks for using this plugin. On the contrary, it makes things easier rather than creating a toc logic from scratch and I too would prefer to use this as well. +1 for adding that 👏

Re the component I was actually thinking about doing something along these lines:

export default function TableOfContents () {
   // ... states here
    useEffect(() => {
       const observer = new IntersectionObserver(
         (entries) => {
           entries.forEach((entry) => {
             // not sure if we use entry.intersectionRatio > 0 we can store in state ?
             // right now when we go backwards it has to reach the heading to trigger active
             // .. something like netlfy docs
             if (entry.isIntersecting) {
                setActiveId(entry.target.id);
             }
           });
         // ...rest
       }
       // ...rest for active class, etc.
       const tocLinks = document.querySelectorAll(".toc-link")
       tocLinks.forEach(item => {
         const id = item.href.split("#").pop()
         observer.observe(document.getElementById(id)
       })
       return () => {
         tocLinks.forEach(item => {
           const id = item.href.split("#").pop()
           observer.unobserve(document.getElementById(id)
         })
         // ...rest
       }
    },[])

    return
}

The pros about above is that it eliminates the need for having the Heading.js component where we do createElement and no modifications in MDXContent.js to the heading. Also, the observer would not be required in <MDXContent observer={observer} ... />.

But I do not know how the performance for the above would stack up compared to your approach, so I think let's just go with yours. 👍

You're right about that this approach prevents us from having a re-usable ToC component, but I don't think it is necessary.

I think so too now. It's best the Sidebar and Toc are separated.

However there is a situation when we could have the ToC on the LHS -> for smaller screens

Yes I agree this would be great a feature to add. Maybe if it's too much for now we can have it in the pipeline.

Concluding, there are a few merge conflicts now since I updated the repo for the SEO issue 😅. I guess that would be all and we can merge this.

EDIT: personally I feel the border looked nice as a separator but that's minor.

Thanks a lot for this !

[olayway]

Hi @khalilcodes

Re the component I was actually thinking about doing something along these lines:

export default function TableOfContents () {
...
}

The pros about above is that it eliminates the need for having the Heading.js component where we do createElement and no modifications in MDXContent.js to the heading. Also, the observer would not be required in <MDXContent observer={observer} ... />.

But I do not know how the performance for the above would stack up compared to your approach, so I think let's just go with yours. +1

I really like this idea! But I couldn't make it work as expected 😞 I'll give it another try again sometime in the future but for now I've moved using the observer hook to the MdxContent component so the MDX component is even cleaner now.

Yes I agree this would be great a feature to add. Maybe if it's too much for now we can have it in the pipeline.

Cool! I'll implement it while working on the LHS ToC (Sidebar).

Concluding, there are a few merge conflicts now since I updated the repo for the SEO issue sweat_smile. I guess that would be all and we can merge this.

Fixed!

EDIT: personally I feel the border looked nice as a separator but that's minor.

I think let's leave it as-is for now. We can test some options in the future and leave the best one.

Additional notes:

1. I've removed dynamic imports for Anchor and Paragraph since (correct me if I'm wrong) they will be loaded no matter what - i.e. there is no conditional rendering of them (no boolean show/hide value or anything like that). I've used it however inside of the Anchor component for dynamic Tooltip import (depending on the type of link in the anchor tag)
2. There are a lot of changes in thepackage.json. I think this is due to the lockfileVersion that has been set to 2 (from 1) by the npm version I'm using (v8). It seems version 1 must have been created by some old npm version. (https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion). Do you know what we could do about this?

[khalilcodes]

I have moved the dates logic to contentlayer config, so these can be safely removed now.

[olayway]

Removed.

[khalilcodes]

Hi @olayway thanks for trying out the component and with the changes.

on Additional notes

1. I've removed dynamic imports for Anchor and Paragraph ...

This was only done for code-splitting for page performance as recommended in lighthouse. In my testing adding dynamic to these had an impact, whereas when I tried this with the Tooltip component there wasn't any difference. Also if we look at the build times (after npm run build) there is a major difference there too. Do have a look at the deploy metrics in #164. But let's test again, and if it affects I will change them back to dynamic.

2. There are a lot of changes in the package.json ...

You are right, I am using npm v6. I'll update it to v8 and hopefully that fixes it.

Other than the small change above, everything looks good to go and we can merge this. 👍

[olayway]

This was only done for code-splitting for page performance as recommended in lighthouse...

I see... I've added the dynamic imports back for now but I have enabled back ssr rendering for Anchor's, because I think we do want them to be visible on the first-page load. If we don't render them on the server-side this is how the page looks like for a short moment (or not so short, depending on the internet connection) before it's fully rendered on the client:

I suspect the metrics improvement had to do with the Tooltip attached to those Anchors. I'll create an issue for that and I'll investigate it. **Issue created: #172 **

[khalilcodes]

@olayway yes, good idea to keep performance testing as a separate issue. Merging this for now. Thanks a lot for the great work !!