-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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): new configuration syntax for a simple footer #6132
feat(theme-classic): new configuration syntax for a simple footer #6132
Conversation
✔️ [V2] 🔨 Explore the source changes: e28c22e 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61c0ca32724e110007523982 😎 Browse the preview: https://deploy-preview-6132--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6132--docusaurus-2.netlify.app/ |
Indeed, you would need to add validation here: https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-classic/src/validateThemeConfig.ts Joi is quite painful and the current PR is already a good start. Do you want to wrap it up or would you prefer me to take it over? |
Cool. Didn't know if Joi schema like that actually works—would you add test cases about validation? |
Very nice! Appreciate the amazing work ❤️ |
Thank you for all the great help! 😄 💯 |
packages/docusaurus-theme-classic/src/__tests__/validateThemeConfig.test.js
Outdated
Show resolved
Hide resolved
Thanks again for the work! |
@slorber you may want to test this locally and check the design before merging, e.g. do we want to center the copyright when the footer is simple? |
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
Did some little modifications but overall that looks good.
For now I kept the simple footer centered above the copyright
}; | ||
|
||
export type MultiColumnFooter = FooterBase & { | ||
links: Array<{ | ||
title: 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.
The config validation allows undefined/null.
Undefined can lead to type guards thinking it's a simple footer despite being a multi column one:
null can lead to a weird rendering alignment:
Not sure who's using null, it may make sense to disable column titles and the alignment may be good enough if none of the columns have titles...
Edit: see use-case for null: #4267
Just adding default(null)
to title validation + this type might be good enough.
Motivation
Fixes #6101
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
I actually had some trouble verifying that the simple footer works, but the multi-column footer still seems to work when viewing
localhost:3000
without changing anything indocusaurus.config.js
.My trouble with simple footer was that I got the following error from
yarn start
when I changed the footer indocusaurus.config.js
to only link items without titles:A validation error occurred. The validation system was added recently to Docusaurus as an attempt to avoid user configuration errors. We may have made some mistakes. If you think your configuration is valid and should keep working, please open a bug report. ValidationError: "footer.links[0].label" is not allowed
It seems like this validation error is being thrown here: https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-utils-validation/src/validationUtils.ts#L92
This is what I changed the footer to in
docusaurus.config.js
https://github.com/facebook/docusaurus/blob/main/website/docusaurus.config.js#L418:Maybe some cache I am missing to clear or some Joi validation I need to change?