Add Table of Contents Component for api pages #1775
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Thanks for opening this PR.
- Please don't increase the font size in this PR, that's a separate issue that requires a dedicated PR, which distracts from the main focus of this one.
- Also, keep the original style for desktop for now. The design you propose looks great for mobile, though some styles need to be adjusted.
- Please don't format the code in areas that are outside the scope of this PR.
Most importantly, we should find a way for Jekyll to generate that table. Moving it to another file might not be the best approach since I'm working on making the API translatable in Crowdin, and managing translations in a separate file would be more complicated.
I'd even be open to using JavaScript to generate the table.
These are my main comments for now. I'll try to review it more thoroughly tomorrow or the day after, but I'm really tired today.
|
Hi @ShubhamOulkar, Thanks for the PR. I was just looking at this and have some concerns.
|
ShubhamOulkar
force-pushed
the
table-of-content
branch
from
a9740ce to
375cdf4
Compare
ShubhamOulkar
force-pushed
the
table-of-content
branch
from
01a160a to
c93833c
Compare
|
@bjohansebas, Thank for your kind suggestions. Now it is ready for review.
|
ShubhamOulkar
changed the title
|
This button should be visible on smaller screens, around 800px, or when the layout actually breaks, not on 1200px screens. |
| if (observer) observer.disconnect(); | ||
|
|
||
| let options = { | ||
| root: null, // Observe relative to viewport | ||
| rootMargin: "-57px 0px 0px 0px", // Slight offset to ensure intersection triggers correctly | ||
| threshold: 0, // Trigger when the element is fully out of view (i.e. behind header) | ||
| }; | ||
|
|
||
| observer = new IntersectionObserver(handleIntersect, options); | ||
| // observe intersection of TOC btn with header bar | ||
| observer.observe(firstHeader); | ||
| } |
There was a problem hiding this comment.
Is it necessary to have an observer? I don't see why we should use it—the less JavaScript, the better. If we can use the menu logic instead, that would be better since this behaves like a menu
We could even use the <details> element for this case. Of course, I'll leave it up to you, but the less JavaScript, the better.
There was a problem hiding this comment.
I agree. Adapt or reusing the menu logic should theoretically get you pretty far, and save you time writing new JS.
|
It shouldn't create that element |
task list
component added into following pages
Related PR #1760
New table of content component design