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): Add proof of concept for theme custom hooks #2714

Closed
wants to merge 1 commit into from

Conversation

fanny
Copy link
Contributor

@fanny fanny commented May 2, 2020

Motivation

as suggested in #2636, when you're creating a new theme, can be interesting have the hooks functionalities to facilitate the components development.

Without this, if a user wants the logic of our sidebar, e.g., he'll need to copy-past the hook.

This is a Proof-of-concept, but i think that we should add a new lifecycle method in the theme, getHooksPath, it seems more consistent.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)
start the classic template

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

@fanny fanny requested review from lex111 and yangshun as code owners May 2, 2020 21:01
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit f08789f

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

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 2, 2020
@fanny fanny added the proposal This issue is a proposal, usually non-trivial change label May 5, 2020
@yangshun
Copy link
Contributor

yangshun commented Jun 5, 2020

Seems fine. @lex111 WDYT? Will this cause any issues? But I don't think they should be added to the @theme alias. Seems like we're abusing the swizzling. How about importing as normal package imports instead?

@slorber
Copy link
Collaborator

slorber commented Jun 25, 2020

Hi,

I think there are multiple scenarios to consider, and this is part of a broader subject.

Sharing code between themes

We might want to share code between the classic and bootstrap themes, and maybe other themes users would build.

To me, this code is not necessarily only hooks, we might share components as well.

For exemple, I think we should share a base MDXComponents between the 2 themes.

So, for me @docusaurus/theme-hooks is not a good name. We will likely want to share more than hooks on the long term, even if now we will mostly share hooks. What about @docusaurus/theme-utils ?

Sharing code between plugin and its theme implementation

If you look at the DocPage component, there's a lot of similarities between the classic and bootstrap theme implementation.

I think we need a way to abstract the shared parts directly in the plugin (that would contain some client-side code).

As the classic theme is now implemented in TS, it would be nice to be able to share some types between the plugin and the theme page, for example the DocMetadata type that the plugin injects as props in the doc page.

We could also share helper functions to help manipulate the injected data, which is not always in the convenient shape for all usecases

@fanny
Copy link
Contributor Author

fanny commented Jun 26, 2020

To me, this code is not necessarily only hooks, we might share components as well.

I agree with you, when we started to build the theme-bootstrap, We decided not create an abstraction, because we could build a wrong abstraction. If you take a look in the bootstrap components, you'll see that the big difference is the styles, because bootstrap have your own language, when I thought in this plugin, I thought in something that could facilitate the life of who is creating themes , so that the user focus on the stylization and and not elsewhere.

Totally agree, I'll refactor this POC.

@fanny
Copy link
Contributor Author

fanny commented Sep 18, 2020

Hey, I was keeping busy because of work. Could I work in this task? I don't know if someone already take it.. or if is in progress

@fanny
Copy link
Contributor Author

fanny commented Sep 18, 2020

I'm thinking in split this in parts to facilitate the review and to help in case of something goes wrong

  1. extract hooks and types

  2. extract some shared logic between the components

  3. extract similar components.

What do you think?

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Hey @fanny .

What I suggest is to create a @docusaurus/utils-theme (or @docusaurus/theme-utils?) and that we move things here one component/hook at a time, so that we can discuss more easily what should be migrated and not migrated to this package. Do you have a good candidate in mind to get started?

An example of an unintuitive bad candidate to me is this one:

image

Despite being duplicated between classic/bootstrap themes, this code is "technical" and does not have anything UI related. In such case I think we should rather find a way to put back this code in docs content plugin directly, so that theme component api surface is simpler and more explicit (eventually find a way to add type safety between content plugin and theme implementation)

@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 fanny/poc-hooks 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 proposal This issue is a proposal, usually non-trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants