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): bootstrap bootstrap (heh) theme, preset, template #2557

Merged
merged 7 commits into from
Apr 9, 2020

Conversation

fanny
Copy link
Contributor

@fanny fanny commented Apr 7, 2020

Motivation

I would like to start a new theme based on bootstrap components. This PR adds the initial structure.

  • Added a bootstrap preset structure
  • Added a bootstrap template structure
  • Added a bootstrap theme

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!)
N\A
I was thinking if is not cool add the prefix EXPERIMENTAL on the folder 🤔

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

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 7, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Apr 7, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 13302da

https://deploy-preview-2557--docusaurus-2.netlify.com

@fanny fanny requested a review from yangshun April 7, 2020 22:40
@fanny fanny added the pr: new feature This PR adds a new API or behavior. label Apr 7, 2020
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let's make the changes and we can merge this

@yangshun yangshun changed the title feat(v2): Add bootstrap theme structure feat(v2): initial bootstrap theme structure Apr 8, 2020
@fanny fanny added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Apr 8, 2020
@fanny fanny removed the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Apr 8, 2020
@yangshun
Copy link
Contributor

yangshun commented Apr 9, 2020

We should split this PR up into smaller ones. I think your original PR containing the theme was the right size, you didn't have to add the template and the preset. We could always do it incrementally:

  1. Add theme
  2. Add preset which uses theme
  3. Add template which uses preset and theme

It's ok, we'll merge this since most of the stuff is content which already exists

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the changes and let you review one final time before we merge this.

EDIT: To prevent conflicts, I'll merge this first. Here's a summary of the changes I made:

  • Renamed package names to @docusaurus/ scope to leverage on monorepo capabilities
  • Removed blog plugin for now as we haven't implemented the theme components.
  • Added the page plugin instead so that we can start the bootstrap template and see something
  • Removed the (currently) unused stuff from the template (docs and blog markup)

Bootstrap template now which only has the index page. Note that the src/pages/index.js also needs to be updated with Bootstrap classes, but that can be done later.

Screen Shot 2020-04-09 at 12 55 17 PM

@yangshun yangshun force-pushed the fanny/add-bootstrap-theme-structure branch from dbef6da to 13302da Compare April 9, 2020 04:20
@yangshun yangshun merged commit 434da1d into master Apr 9, 2020
@yangshun yangshun changed the title feat(v2): initial bootstrap theme structure feat(v2): bootstrap bootstrap (heh) theme, preset, template Apr 9, 2020
@yangshun yangshun deleted the fanny/add-bootstrap-theme-structure branch April 9, 2020 05:00
@lex111
Copy link
Contributor

lex111 commented Apr 9, 2020

Shouldn't we encourage people to use our own CSS framework of Infima? Also, if the Bootstrap theme is the core package, it means we have to support +1 theme, right? I do not quite understand the meaning of this 🤷‍♂️.

@yangshun
Copy link
Contributor

yangshun commented Apr 9, 2020

Shouldn't we encourage people to use our own CSS framework of Infima?

Yes but we also don't want people to be locked into our CSS framework if they don't feel like it and give them the option to choose their own CSS framework. I believe this is better for the community in the longer term.

Also, if the Bootstrap theme is the core package, it means we have to support +1 theme, right?

For now it'll be maintained by us but in future it could be moved out and maintained by the community if we find that we don't have the bandwidth to support it. It's also a good chance and test for us to evaluate how clean our architecture is. If we can easily implement a new theme without changing anything in@docusaurus/core, that means our architecture is solid. It's also a way to identify the common modules we can extract out, e.g. the code block stuff or algolia stuff. Overall there are more benefits than downsides in the near term IMO.

@fanny
Copy link
Contributor Author

fanny commented Apr 9, 2020

I agree that can be problematic, if we need to give support for many themes. But as yangshun said, I see a good way to evaluate and think in ways that can allows us to be more extensible and flexible.

Is common that the users need other visual identities, if we don't provide ours, they'll create their own, is more to have standardised components, but in a near future, we can provide a showcase of themes builted by community.

@lex111 lex111 removed the pr: new feature This PR adds a new API or behavior. label Apr 27, 2020
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.

5 participants