-
Notifications
You must be signed in to change notification settings - Fork 515
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(blog): add toc #9707
feat(blog): add toc #9707
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly looks good from my slightly time pressured review, mostly nits, give it a good test before deploying to prod!
<aside className="toc"> | ||
<nav> | ||
{doc.toc && !!doc.toc.length && <TOC toc={doc.toc} />} | ||
</nav> | ||
</aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want all of this to be conditional on doc.toc && !!doc.toc.length
, or do this aside and nav need to be here permanently for the css to work with the placement properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a copy of the article toc. And I'm already touching to many things here, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for large screens for now. This also refactors the useStickyHeaderHeight hook.
Co-authored-by: Leo McArdle <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'm still experiencing the mentioned issues:
Screen.Recording.2023-09-29.at.00.20.22.mov
Sorry I fixed that now. |
Avoids sidebar being inside the main-page-content, to be consistent with document pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a fix (73d492f, the TOC links were not readable while clicking on them, due to .main-page-content
style being applied to them), now it looks good.
Summary
Display a ToC on blog posts.
This also refactors the useStickyHeaderHeight hook.
Problem
Hard to navigate long blog posts.
Solution
Reuse the ToC component with some tweaks.
Screenshots
Before
After
How did you test this change?
Locally