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
refactor(docs,theme): split DocItem comp, useDoc hook #7644
Conversation
- put doc route data in context - access data through useDoc() hook - split components into much smaller subcomponents
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
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.
Waiting for a review before merge
/** | ||
* Decide if the toc should be rendered, on mobile or desktop viewports | ||
*/ | ||
function useDocTOC() { |
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.
maybe we'll want to extract this hook later?
Not 100% satisfied with it atm
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.
I don't like this—seems like over-abstraction that hides its semantics.
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.
What would you suggest?
I'd like the layout component to be easy to edit visually, without the TOC technical details noise.
Maybe it's fine to keep this weird hook, but just keep it there so that users can understand it more easily? 🤷♂️
I don't like it either but I don't have much better to suggest without polluting the component
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.
Yes, when I'm swizzling, I'd personally like to gain some understanding of what the hook is doing. This is especially important for non-TS users. This hook is congregating too many unrelated things. It's fine if it's in the same file and easily viewable, but extracting it to theme-common
makes it much harder to work with.
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.
agree 👍
let's ship this as is and see how to improve later
* but we may want to change that in the future. | ||
* This layer is a good opportunity to decouple storage from consuming API. | ||
*/ | ||
function useContextValue(content: PropDocContent): DocContextValue { |
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.
My opinion: storing metadata/frontmatter in the mdx file is an implementation detail. Consuming the data with Component.metadata
doesn't feel very natural to me so I'd rather not stick to it 🤷♂️
In practice, we could create separate bundles/props for these metadata. And it may have advantages to do so in the future if for example we want to provide some kind of "data query layer" (I mean, for example we want to display some docs/blog metadata in the homepage, without having to load all the mdx content in that page)
I'm not sure how things will turn out, but I'd prefer to decouple the metadata from the MDX component when consuming it. I'd like to do something similar on the blog as well.
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.
I agree—the metadata should be a separate chunk. This means we can easily provide the same metadata as both props and route context. Seems too complex in the short term, though.
Size Change: +18 B (0%) Total Size: 801 kB ℹ️ View Unchanged
|
function useContextValue(content: PropDocContent): DocContextValue { | ||
return useMemo( | ||
() => ({ | ||
MDXComponent: content, |
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 to expose this as context? Looks like TMI to me 😄
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 part of the static data being available in the subtree, so not sure how it could be used, but why not?
Eventually we may move all this subtree context data to be made available at the plugin level? Using our secret useRouteContext
? #6885
(I mean useDoc()
➡️ useRouteContext().doc
, no need for DocContext
)
You'd prefer to forward a children
prop?
I think both solutions are fine 🤷♂️ it's unlikely someone would want to render that component in a weird place like the Footer, so forwarding children
could also work
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.
I was thinking this is useless in all cases to be exposed as a context, since only the main container would care about it and it doesn't really encourage any code reuse. (e.g. no one would be forwarding this to some subcomponent as props.)
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.
Up to you, I'm fine if you revert it to children
if you think it's better, both look fine to me
return useMemo( | ||
() => ({ | ||
metadata: content.metadata, | ||
frontMatter: content.frontMatter, | ||
assets: content.assets, | ||
contentTitle: content.contentTitle, | ||
toc: content.toc, | ||
}), |
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.
Are we intentionally cherry-picking the properties from content
on both type and value, instead of using a spread?
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.
yes because I'd like to avoid mdx loader export implementation details to automatically leak into an API that we'll likely make public later
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.
Makes sense. Just confirming
@slorber is
|
@lanegoolsby Was looking into utilizing this hook as well, and from preliminary investigation seems like the useDoc hook is only being exported under the import { useDoc } from '@docusaurus/theme-common/internal' |
@lanegoolsby You need to compile with |
Can this hook be used in a custom plugin? I can use it in a swizzled TOC, but I get this error when I try to use the same code in a custom plugin. I am using a I tried both with and without a
|
I'm not sure to understand at all what you are trying to do here 😅 please show a repro. Note technically you could reuse Note: you can't call the |
I'm trying to setup a custom plugin that injects a custom React component immediately below the Table of Contents component, for example: Where the Hello World is, I'd like to have a component that will read all the tags for a doc page, query an internal API with said tags via HTTP/REST, and render the results in a grid. |
Note the TOC component is more a "design system component" and it's not really meant to be coupled to a specific usage (docs, blog, pages). I mean it's not supposed to use If you only want to enhance the Doc TOC (and not the page/blog TOC), you should rather swizzle Also don't forget to think about the mobile experience where the TOC is a collapsible at the top of the document |
Breaking change
@theme/DocItem
component has been split into much smaller subcomponents@theme/DocItemFooter
has been moved to@theme/DocItem/Footer
and does not receive props anymoreuseDoc()
hookPre-flight checklist
Motivation
This is a follow-up of moving in the direction to:
We already did that in multiple places as well (doc sidebar etc...)
This PR does:
useDoc()
hook, that will likely become part of our public APIWe'll likely mark some of these components as safe to swizzle later?
Test Plan
Preview works as before
Test links
Deploy preview: https://deploy-preview-7644--docusaurus-2.netlify.app/docs
Related issues/PRs
Somehow related: #7637 (useDoc() will make this easy)