-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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(content-docs): refactor sidebars, Joi validation, generator rework, expose config types #5678
Conversation
205a15d
to
e6c6b64
Compare
✔️ [V2] 🔨 Explore the source changes: 04a8f6a 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/616645ed59ce8200073037bb 😎 Browse the preview: https://deploy-preview-5678--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5678--docusaurus-2.netlify.app/ |
1c227fe
to
330be5c
Compare
4c19262
to
c2e7d94
Compare
c2e7d94
to
dcab14f
Compare
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 👍 thanks for the cleanup
Not easy to review since many files have no diff 🤪 but overall the behavior looks similar
module.exports = { | ||
// @ts-check | ||
|
||
/** @type {import('@docusaurus/plugin-content-docs').SidebarsConfig} */ |
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.
nice idea :)
} | ||
return item; |
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 category child items still processed? It seems not and we probably lack a test here
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.
oh I see it's using the recursive transformSidebarItems, looks fine
@@ -116,11 +117,9 @@ export default function pluginContentDocs( | |||
getPathsToWatch() { | |||
function getVersionPathsToWatch(version: VersionMetadata): string[] { | |||
const result = [ | |||
...flatten( |
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.
👍 in Node I think it's safe to use flatMap/flat now (wasn't the case before but we can refactor and remove usage of lodash everywhere)
Yeah, after the code splitting I think we have a lot more places to add tests on, but I'm not good at writing unit tests, so let's leave it till we implement the next feature |
Motivation
Followup of #5642 (comment). Hope this would make further work on sidebars easier.
Loading a sidebar follows the
validation
->normalization
->processor
workflow, and the type goes fromSidebarsConfig
->NormalizedSidebars
->Sidebars
.A few types are renamed, and the config types are exposed to allow type-checking. Not sure if that would be BC
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Passes all unit tests + website build