-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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(core,theme): useRouteContext + HtmlClassNameProvider #6933
Conversation
@@ -0,0 +1,59 @@ | |||
/** |
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.
temp because the other PR also have created a similarly named file
I've seen this error in the past, when I was trying to switch the JS loader from esbuild to SWC. Has something to do with CJS/ESM. Haven't had time to debug this, since the stack tracing in SSR is horrible |
Removed the calls to
Which seems reasonable |
The reason that there's now this CJS/ESM interop problem is because I removed the Non-babel loaders always have this or that kinds of failures... |
✔️ [V2] 🔨 Explore the source changes: 89978ea 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6233e75a1e054400080098bf 😎 Browse the preview: https://deploy-preview-6933--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6933--docusaurus-2.netlify.app/ |
Size Change: +9.49 kB (+1%) Total Size: 802 kB
ℹ️ View Unchanged
|
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!
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.
thanks 👍
loader: async () => { | ||
const NotFound = (await import('@theme/NotFound')).default; | ||
return (props) => ( | ||
// Is there a better API for this? |
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.
🤪 good catch
There's also a native "Loading" screen but afaik it's never displayed so 🤷♂️
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.
Yeah, the "native" name isn't the best, but I don't think this will be targeted anyways...
@@ -30,6 +30,7 @@ function mergeContexts({ | |||
return value; | |||
} | |||
|
|||
// TODO deep merge this |
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.
not sure we really want to deep merge
In case there are multiple layers I'd rather ensure that laters do not override each others, there's no good use-case to do so that I can think of
We can keep the comment for now and figure this out later anyway
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 think it would be immediately useful for content plugins, but maybe in userland? Deep merging sounds more natural about how these stores should behave
Co-authored-by: Joshua Chen <sidachen2003@gmail.com> # Conflicts: # packages/docusaurus-theme-common/src/index.ts
Motivation
Add plugin name + plugin id + docs version + doc id to HTML classes, fixes #4280
Partial implementation for assigning a context to a route (#6885)
For now only plugins add basic data to this context, and it can be used through
useRouteContext()
. For now I keep this API undocumented until we have a full clean implementation.This partial implementation was necessary because there was no other easy way to assign pluginId/name to a given route apart from doing more annoying things like adding data to the generated route file.
Had to create a
HtmlClassNameProvider
wrapper because react-helmet does not "merge" classNames and deeply nested class override parent classes (see staylor/react-helmet-async#161). Another option would be to use data-attributes instead of classes for parent selectors? 🤷♂️Note that some things are unpolished, but I plan to do more cleanup/merge as part of #6925 (2 smaller PRs instead of one bigger) so I'll merge this one asap to unlock the other reactors. Now is the best time to review.
Have you read the Contributing Guidelines on pull requests?
yes
Test Plan
Not so easy to test 😅
Preview builds and applies correct classNames
Related PRs
#6925