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

Material-UI and Ant Design requires a plugin #19277

Merged
merged 3 commits into from
Dec 10, 2019

Conversation

oliviertassinari
Copy link
Contributor

  • Ant Design needs a plugin to handle tree shaking and to import the styles.
  • Material-UI needs a plugin to handler server-side style sheets, with JSS. In the future, we will rely on gatsby-plugin-styled-components.

The problem was reported at mui/material-ui#18197. It was introduced in #6099.

You might have noticed that I have switch the order between the two libs, of course, it's not biased, happy to follow the alphabetical ASC order if you prefer 🙃

- Ant Design needs a plugin to handle tree shaking and to import the styles.
- Material-UI needs a plugin to handler server-side style sheets, with JSS. In the future, we will rely on gatsby-plugin-styled-components.

*First reported in mui/material-ui#18197
*You might have noticed that I have switch the order between the two libs, of course, it's not biased, happy to follow the alphabetical ASC order if you prefer 🙃*
@@ -7,7 +7,7 @@ Most third-party functionality you want to add to your website will follow stand
Some examples:

- Importing JavaScript packages that provide general functionality, such as `lodash` or `axios`
- Using React components or component libraries you want to include in your UI, such as `Ant Design`, `Material UI`, or the typeahead from your component library.
- Using React components or component libraries you want to include in your UI or the typeahead from your component library. ⚠️ You need a plugin for [Material-UI](https://github.com/hupe1980/gatsby-plugin-material-ui) or [Ant Design](https://github.com/bskimball/gatsby-plugin-antd).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if we need plugins for those libraries, then whole bullet point about packages that don't need dedicated gatsby plugin loses a lot of sense ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe the documentation for plugins should bring clarity on situation where users need them. Because, getting a dependency from npm is always required. It is not clear how plugins help. A good idea might be to add a diagram to visualize why of plugins in Gatsby.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove the bullet point?

Copy link

Choose a reason for hiding this comment

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

Yes, I believe the documentation for plugins should bring clarity on situation where users need them. Because, getting a dependency from npm is always required. It is not clear how plugins help. A good idea might be to add a diagram to visualize why of plugins in Gatsby.

Totally support this . It's quite a struggle to know as of now that the plugins are mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay in responding to this @oliviertassinari! I'm in favor of removing the bullet, but think it might make sense to clarify the nuance with something like this:

Suggested change
- Using React components or component libraries you want to include in your UI or the typeahead from your component library. ⚠️ You need a plugin for [Material-UI](https://github.com/hupe1980/gatsby-plugin-material-ui) or [Ant Design](https://github.com/bskimball/gatsby-plugin-antd).
- Using React components or component libraries you want to include in your UI or the typeahead from your component library. ⚠️ In some cases with these libraries you may need a plugin to handle more advanced use cases like server-side rendering and adding style providers with [Material-UI](https://github.com/hupe1980/gatsby-plugin-material-ui), or to configure Babel for you like with [Ant Design](https://github.com/bskimball/gatsby-plugin-antd).

This might be a compromise to show that I could import some really simple generic component I've made and published myself, or include one of these more impressive libraries that needs to cover more edge cases and provide more customization.

Maybe @pieh has more thoughts too? It does seem a little silly to name a guide "what you don't need plugins for" and then need a plugin for one of the examples 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 for removing the bullet.

@LekoArts LekoArts added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response type: documentation An issue or pull request for improving or updating Gatsby's documentation labels Nov 18, 2019
@LekoArts LekoArts added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Dec 2, 2019
@laurieontech
Copy link
Contributor

Hey @oliviertassinari! Looks like everyone is in agreement here. Are you able to accept and commit these changes so we can get this merged in?

@oliviertassinari
Copy link
Contributor Author

@laurieontech Sorry, what direction are we taking? Do you want to push directly on my fork?

@laurieontech
Copy link
Contributor

@oliviertassinari I'd go for @gillkyle's suggestion. I was going to let you make the changes as long as you're in agreement with them.

@laurieontech
Copy link
Contributor

Awesome! Thanks for pointing this out and talking through the options. Hopefully, this will remove the confusion people were having.

@laurieontech laurieontech merged commit 823988d into gatsbyjs:master Dec 10, 2019
@gatsbot
Copy link

gatsbot bot commented Dec 10, 2019

Holy buckets, @oliviertassinari — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants