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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core,theme): useRouteContext + HtmlClassNameProvider #6933

Merged
merged 7 commits into from Mar 18, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Mar 17, 2022

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

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Mar 17, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 17, 2022
@@ -0,0 +1,59 @@
/**
Copy link
Collaborator Author

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

@slorber
Copy link
Collaborator Author

slorber commented Mar 17, 2022

Not sure why it doesn't build 🤔 any idea?

image

@Josh-Cena
Copy link
Collaborator

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

@Josh-Cena Josh-Cena changed the title feat(core,theme): add useRouteContext + HtmlClassNameProvider feat(core,theme): useRouteContext + HtmlClassNameProvider Mar 18, 2022
@Josh-Cena
Copy link
Collaborator

Removed the calls to logger in serverEntry.tsx because it seems like logger was not called correctly. The error is:

Docusaurus server-side rendering could not render static page with path path=/404.html.


Error: Unexpected: no Docusaurus parent/current route context found

Which seems reasonable

@Josh-Cena
Copy link
Collaborator

The reason that there's now this CJS/ESM interop problem is because I removed the format: 'cjs' in #6752 (diff). I removed that because of evanw/esbuild#2037 which made our site not able to build at all 😅

Non-babel loaders always have this or that kinds of failures...

@netlify
Copy link

netlify bot commented Mar 18, 2022

✔️ [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

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 55
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6933--docusaurus-2.netlify.app/

@github-actions
Copy link

Size Change: +9.49 kB (+1%)

Total Size: 802 kB

Filename Size Change
website/build/assets/css/styles.********.css 105 kB -247 B (0%)
website/build/assets/js/main.********.js 608 kB +9.7 kB (+2%)
website/build/index.html 38.8 kB +40 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB

compressed-size-action

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator Author

@slorber slorber left a 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?
Copy link
Collaborator Author

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 🤷‍♂️

Copy link
Collaborator

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
Copy link
Collaborator Author

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

Copy link
Collaborator

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

@slorber slorber merged commit 8a1421a into main Mar 18, 2022
@slorber slorber deleted the slorber/useRouteContext branch March 18, 2022 09:57
slorber added a commit that referenced this pull request Mar 18, 2022
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
# Conflicts:
#	packages/docusaurus-theme-common/src/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a unique CSS class to body tag which uses the docs ID
3 participants