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

refactor(v2): improve and unify theme config types #4433

Closed
wants to merge 1 commit into from

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Mar 14, 2021

Motivation

i'v tried making this change minimal, and not touch to much of code, but going from any to proper type showed some errors

- key={isClient}
+ key={String(isClient)}

#4385 (comment)

Have you read the Contributing Guidelines on pull requests?

yes

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 14, 2021
@armano2 armano2 changed the title refactor(v2): improve and unify theme config refactor(v2): improve and unify theme config types Mar 14, 2021
@armano2 armano2 force-pushed the refactor/improve-theme-types branch from 366fbe6 to 6a353d8 Compare March 14, 2021 11:35
@netlify
Copy link

netlify bot commented Mar 14, 2021

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit 366fbe6

https://deploy-preview-4433--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Mar 14, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 100
🟠 Accessibility 72
🟢 Best practices 93
🟠 SEO 50
🟠 PWA 55

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

@netlify
Copy link

netlify bot commented Mar 14, 2021

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit db2171d

https://deploy-preview-4433--docusaurus-2.netlify.app

@netlify
Copy link

netlify bot commented Mar 15, 2021

@netlify
Copy link

netlify bot commented Mar 15, 2021

Copy link
Collaborator

@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 for the cleanup :)

There's something that bothers: you moved all the theme types to @docusaurus/types but this type package is meant to be used for core types, not plugin/theme types, as these types are specific to one theme in particular, but some users might want to use Docusaurus with a totally different theme with different types. That's why {} is a valid/default type and the core ThemeConfig should remain Record<string,any> or something similar. Note it's possible to use Docusaurus with multiple themes at the same time, and we end up with a composition where the final theme config is the theme config of the 2 underlying themes (ClassicThemeConfig & LiveCodeBlockThemeConfig)

In practice, the plan is to share a common ThemeConfig type across multiple themes (classic/bootstrap/tailwind), but keep the core agnostic of those types, that's why the types were put in @docusaurus/theme-common

For now, it's not necessary to take any care of the bootstrap theme, it is not ready for usage at all.

@armano2
Copy link
Contributor Author

armano2 commented Mar 15, 2021

ok, I'm going to update this with module augmentations

@netlify
Copy link

netlify bot commented Mar 16, 2021

@netlify
Copy link

netlify bot commented Mar 16, 2021

@armano2 armano2 force-pushed the refactor/improve-theme-types branch from 7532e80 to db2171d Compare March 19, 2021 11:32
@Josh-Cena
Copy link
Collaborator

Closing because of how quickly things have evolved. A lot of things have already been addressed. Thanks @armano2 anyways :D

@Josh-Cena Josh-Cena closed this Oct 28, 2021
@armano2 armano2 deleted the refactor/improve-theme-types branch November 3, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants