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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Table of Contents #1

Closed
wants to merge 8 commits into from
Closed

Conversation

karenying
Copy link
Owner

@karenying karenying commented Oct 30, 2020

This PR adds a sticky table of contents to the right of posts. It's only visible for screen sizes of $layout-breakpoint-lg (1100px) and up (I considered shoving it under the title for smaller screens but think it looks too much of a long boi).

On MBP 13":
122995655_632055407678311_123198140323087338_n

In Post.js, I pass in the html string to the component. Then I create an html document element in order to traverse the DOM tree and grab the header elements. For each of header elements, I remove their one child (which was the gatsby-remark-autolink-headers anchor. Then I wrap the header node in an <a> tag, with the header's id as the href. IMPORTANT: you have to remove the id attribute of the header or else gatsby-remark-autolink-headers breaks since it'll use the links in this new component. Finally, I dangerouslySetInnerHTML this generated string. Honestly, not very elegant at all 馃槵

Currently, the CSS only supports up to h4 because I don't anticipate using smaller headers than that (don't even think I use h4s anywhere yet).

Iffy about:

  • just if this is the right way to go about it?????
  • text-shadow hover -- it's not super apparent; maybe underline is better?
  • max-width/max-height/positioning of component (I did a jank calculation)

Copy link

@vzhou842 vzhou842 left a comment

Choose a reason for hiding this comment

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

overall this approach is fine and it works but it's a little problematic from a perf perspective IMO

you're passing down html through props in your React and then dynamically building an entire new DOM representation of your current page during each rerender of TableOfContents just to parse it and find the header tags, then dynamically render some stuff to make them a TOC

This can become really suboptimal if you have a really long post or a not-so-great computer. Instead, you could try a different approach:

  • [not great] just parse the html string directly instead of doing document.createElement and making an entire new DOM representation of the page. This is still not great because you still have to pass down the html string, seems like a big waste, also parsing the string directly can be complicated / expensive still

  • [best?] generate the TOC at build time. I haven't looked into the specifics of this yet but it definitely should be possible, and I think this is the "optimal" solution. You push all the heavy computation (generating TOCs) to build time, and at actual runtime everything is very light: you only pass down the minimum data needed to render TOC to the client (very little data, just some strings really) and the render is extremely cheap.

const AStart = `<a href=#${id} >`;
const AEnd = '</a>';

node.removeAttribute('id');

return AStart + node.outerHTML + AEnd;

Choose a reason for hiding this comment

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

nit this could all have just been one template string that reads clearer

return `<a href=#${id}>${node.outerHTML}</a>`;

@karenying karenying closed this Dec 5, 2020
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.

None yet

2 participants