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

Core: Allow custom postcss config #8498

Merged

Conversation

thollander
Copy link
Contributor

@thollander thollander commented Oct 21, 2019

  • Search for the nearest postcss config
  • Maintain previous defaults if no postcss config

Issue: #2455
Old PR: #4411

What I did

  • Add find-up to dependencies to search for the nearest postcss.config.js
  • Use the nearest postcss config (.postcssrc.json, postcss.config.js...) to determine postcss-loader options if present, maintain previous defaults if not.

How to test

Run the example storybook/examples/postcss-kitchen-sink.
The h1 should have the color: #639; instead of color: rebeccapurple;.

  • Is this testable with Jest or Chromatic screenshots?
    No
  • Does this need a new example in the kitchen sink apps?
    I created one, not sure if it was mandatory.
  • Does this need an update to the documentation?
    Yes, I added a documentation in /custom-postcss-config

- Search for the nearest postcss config
- Maintain previous defaults if no postcss config
@vercel
Copy link

vercel bot commented Oct 21, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/1uqipq3zs
🌍 Preview: https://monorepo-git-fork-thollander-feature-use-custom-postcss-config.storybook.now.sh

Copy link
Member

@shilman shilman 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 putting this together @thollander !

A few thoughts:

  • Would it be possible to add the example to an existing examples/ project rather than creating a new one? I don't want to increase our build times if possible.
  • Right now postcss support is implemented in core, but I'm wondering if we can move it into a preset? Any chance you might be able to take this on? I'd like to move as much as possible out of core to make the default storybook experience lean and mean. This would be a breaking change, so the first step would be to move this into a preset and always include that preset. In a subsequent release (6.0?) we'll make it easy for users to opt-in to those presets

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Ace 🎯

@thollander
Copy link
Contributor Author

Would it be possible to add the example to an existing examples/ project rather than creating a new one? I don't want to increase our build times if possible.

Yes for sure, I did it. You’re totally right it is not necessary to increase the build time just for this. 

Right now postcss support is implemented in core, but I'm wondering if we can move it into a preset? Any chance you might be able to take this on? I'd like to move as much as possible out of core to make the default storybook experience lean and mean. This would be a breaking change, so the first step would be to move this into a preset and always include that preset. In a subsequent release (6.0?) we'll make it easy for users to opt-in to those presets

I get your point but not sure how to implement it yet. I’ll give it a try. 
If you’re ok with theses changes you can still merge this PR.
If I succeed to manage as a preset I would open another PR. 

@shilman shilman changed the title feature(postcss): Allow custom postcss config Core: Allow custom postcss config Oct 23, 2019
@shilman shilman merged commit 5d967df into storybookjs:next Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants