-
-
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(theme-classic, plugin-docs): sidebar item level-specific className + allow customization #5642
Conversation
✔️ [V2] 🔨 Explore the source changes: 5f25f31 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/615a555f78e9b7000773731b 😎 Browse the preview: https://deploy-preview-5642--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5642--docusaurus-2.netlify.app/ |
I feel like the sidebar logic needs decent refactoring, currently 600 + 300 lines of code and adding one property requires patching ten files. |
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 👍
throw new Error( | ||
`Error loading ${JSON.stringify(item)}: "label" must be a string.`, | ||
); | ||
} | ||
|
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 urgent but we should probably make a Joi schema for all these older validations
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, that would be part of the "refactoring" :P
@@ -80,7 +80,7 @@ function DocSidebarDesktop({path, sidebar, onCollapse, isHidden}: Props) { | |||
!isAnnouncementBarClosed && showAnnouncementBar, | |||
})}> | |||
<ul className={clsx(ThemeClassNames.docs.docSidebarMenu, 'menu__list')}> | |||
<DocSidebarItems items={sidebar} activePath={path} /> | |||
<DocSidebarItems items={sidebar} activePath={path} level={1} /> |
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.
wonder if level 1 shouldn't be a default for convenience?
Also a new required prop on a theme component can be considered as a breaking change, but in practice it probably won't affect many people on upgrade
Not sure what you mean by 600 + 300. What kind of refactoring do you have in mind? |
Splitting into more files; simplifying the autogeneration algorithm (which is also part of the TODO); use Joi, etc. |
As I'll start working on category index pages, if you want to submit a refactor it would be better to receive the PR for it soon ,or we'll do the refactorings afterward |
I can do the refactor after that. Maybe also add "level: 1" as a default prop as you work on the category index? |
Motivation
Resolve #5624.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Dogfood