refactor(v2): add useThemeConfig hook + cleanup useless theme default values #3394
Conversation
|
Deploy preview for docusaurus-2 ready! Built with commit 65b262a |
|
@slorber tests are failing. Looks like I've done it the wrong way! |
packages/docusaurus-theme-classic/src/theme/AnnouncementBar/index.tsx
Outdated
Show resolved
Hide resolved
|
Fixed it! Is it good? @slorber |
slorber
changed the title
There was a problem hiding this comment.
Thanks @imskr that almost look good.But there is a place where you read title/scroll config from themeConfig instead of navbar.
According to the current theme validation config (lack of required/default value), navbar can be null. Can you test whether or not it is possible to run a docusaurus site (on master) with navbar = null in config? If the site fails, we can safely add a required() to the Joi schema here and prevent this mistake, but if it works, we should maybe account for this and ensure that we don't destructure navbar before a null check. Config errors should all be thrown ahead of time with validation system.
Also, I think it's worth creating a "useClassicThemeConfig" hook to simplify a bit the code. Can you add it and type it with any returntype for now? We'll figure out to type it better after and how to keep it in sync with the Joi schema in another PR (you can put a // TODO improve typing)
| } = {}, | ||
| isClient, | ||
| } = useDocusaurusContext(); | ||
| const {title, hideOnScroll} = useDocusaurusContext().siteConfig.themeConfig; |
There was a problem hiding this comment.
This looks wrong to me, as before the title/hideOnScroll should be read on navbar, not themeConfig.
(take care, navbar can be null)
| } = {}, | ||
| } = useDocusaurusContext(); | ||
| hideOnScroll = false, | ||
| } = useDocusaurusContext().siteConfig.themeConfig.navbar; |
There was a problem hiding this comment.
navbar can be null (see Joi validation, there is no .require() call nor .default() so it is optional)
| title, | ||
| items = [], | ||
| hideOnScroll = false, | ||
| } = useDocusaurusContext().siteConfig.themeConfig.navbar; |
|
Hi @imskr . Do you want me to complete this PR and merge it? |
Sorry couldn't find time. Yes, please Thank you |
|
thanks @imskr I finished the work and merging the PR now |
slorber
changed the title
slorber
added
the
pr: maintenance
Motivation
packages/docusaurus-theme-classic.Have you read the Contributing Guidelines on pull requests?
Test Plan
yarn test docusaurus-theme-classicRelated PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)