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

feat(v2): create common base theme #3752

Closed
wants to merge 2 commits into from
Closed

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 14, 2020

Motivation

Originally I actually wanted to fix #3747, for which I just created new webpack alias for theme utils (see first commit 16e09fc). However, then I realized that this is not a good solution in the long term. Thus, I have to came that we need to create new package for the common/shared/reusable parts of any Docusaurus theme.
Similar subject has already been raised in PR #2714. However, in current PR I am only moving the theme utils (mostly to resolve #3747), but this is good starting point for further work to split classic theme.

But, technically, I decided to do this via slightly different approach (opposite of #2714) - a really used theme (note, not preset) can have a base theme, i.e. it will be based on its components/hooks/etc. (Maybe it sounds a little confusing, and we need to think about the better naming/definition). In fact, base theme is essentially a regular plugin theme, which is give reusable parts for using in actual theme (and in subsequent themes too).

This functionality works in the case of using a preset (generally it is use case by default) and if only the theme is used (without preset). Since not everyone uses the classic preset, they can still use base theme's parts. I almost forgot: base/common theme adds webpack alias theme-utils, I thought it would be handy for all us.

It's more of a POC to show my whole idea. Perhaps the code needs to be improved or modified in some way (not a big deal, though).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview. And of course the command yarn swizzle @docusaurus/theme-classic Footer runs without errors anymore.

Related 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.)

@lex111 lex111 requested a review from slorber as a code owner November 14, 2020 22:04
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 14, 2020
@netlify
Copy link

netlify bot commented Nov 14, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 08308a0

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

@lex111
Copy link
Contributor Author

lex111 commented Nov 14, 2020

It's strange, E2E tests fails, I missed something?

@slorber
Copy link
Collaborator

slorber commented Nov 16, 2020

Thanks @lex111 , we really need a way to share frontend code this way, not only to fix the existing issue, but also to expose less things to swizzle, and share common non-ui code with upcoming Bootstrap/tailwind themes.

Unfortunately, it's not a very good time to work on this, as it's likely to conflict with my i18n branch.
Also I'm not very fan of using too much webpack aliases everywhere, as it makes the whole system more implicit and complex.

I think we should rather just use regular imports, and let the shared code emit its own TS typedefs instead, like a regular third-party TS package.

We don't need users to swizzle content of this package: they can already swizzle the theme components that import from this package instead.

Let's talk on Discord soon to discuss about all that

@lex111
Copy link
Contributor Author

lex111 commented Nov 16, 2020 via email

@slorber
Copy link
Collaborator

slorber commented Nov 18, 2020

closing in favor of @docusaurus/theme-common: #3775

@slorber slorber closed this Nov 18, 2020
@lex111 lex111 deleted the lex111/theme-utils-alias branch November 18, 2020 16:27
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.

#3394 appears to have broken swizzle
3 participants