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

Add an empty PostCSS configuration option to the "new" command #190

Merged

Conversation

ayushn21
Copy link
Member

@ayushn21 ayushn21 commented Nov 25, 2020

This is a 🙋 feature or enhancement.

  • I've added tests (if it's a bug, feature or enhancement)
  • I've adjusted the documentation (if it's a feature or enhancement)
  • The test suite passes locally (run script/cibuild to verify this)

Summary

As discussed in #29, this PR adds a barebones PostCSS configuration to bridgetown new via a use-postcss flag.

I've left the logic for the directory configuration in the commands/new.rb file for now as I feel extracting it into its own file seems a bit overkill for now. If we add more options to bridgetown new though we will need an AppBuilder or similar sooner rather than later I reckon. If you think it's worth extracting now, I'm happy to proceed based on feedback.

Context

Resolves #29

@jaredcwhite
Copy link
Member

Thanks so much @ayushn21 for working on this! Give me a bit of time to try it out with a few example scenarios and I'll let you know if I have any feedback. :)

@ayushn21
Copy link
Member Author

Sure, take your time :). Cheers!

@jaredcwhite
Copy link
Member

@ayushn21 I dug into this a bit…great job working this in, but I'm quickly running into issues regarding the Sass support via PostCSS and the new Dart-based Sass implementation which apparently is totally incompatible with how I like to write Sass. :( (aka switching from a global namespace for variables to using @use for scoped variables — messes up how Bulma currently works, for example, although I think Jeremy is currently working on updates to support Dart-based Sass. Similar issues abound with Bootstrap 5 as well.)

So my preference at this point would be to:

  1. Switch things around so the current Sass-based implementation is the default and make the PostCSS config an option.
  2. Remove Sass from the PostCSS configuration altogether…if people want to add Sass support to the PostCSS setup they can do so manually (and I'm fine with providing extra documentation to point them in the right direction).

Tailwind, for example, doesn't rely on Sass at all and sort of discourages it, so if we have a "clean room" PostCSS config ready to go and folks can add on Tailwind in a matter of minutes, that's a huge win.

@ayushn21
Copy link
Member Author

ayushn21 commented Dec 4, 2020

Thanks for spending time on this @jaredcwhite.

Yeah even I've found in the last few days that PostCSS based Dart-Sass isn't as similar to traditional Sass as I'd have liked/hoped. It does work great within my setup though. Among other things, I've largely swapped Sass variables for native CSS variables because I like how they cascade.

I completely agree with your proposed approach. An empty PostCSS config would accomplish all the boilerplate stuff so a user could just plug in whatever they wanted and rely on docs/blog posts for guidance.

Do you think it would be worth having some plugins in the default PostCSS config though? I was thinking postcss-flexbugs-fixes and postcss-preset-env. These are what you get in the Rails 6 default template as well.

I'll spend some time on this next week and rework it as you've proposed :)

@jaredcwhite
Copy link
Member

That all sounds good to me. I too have gotten excited about using native CSS variables a lot!

@ayushn21 ayushn21 marked this pull request as ready for review December 8, 2020 18:15
@ayushn21 ayushn21 changed the title Change default css processor to PostCSS and add use-sass option for bridgetown new Add an empty PostCSS configuration to bridgetown new via a --use-postcss flag Dec 8, 2020
@ayushn21 ayushn21 changed the title Add an empty PostCSS configuration to bridgetown new via a --use-postcss flag Add an empty PostCSS configuration option to the "new" command Dec 8, 2020
@ayushn21
Copy link
Member Author

ayushn21 commented Dec 8, 2020

@jaredcwhite I think this is more or less good to go.

If you have any feedback about the code, let me know! Happy to make updates!

@jaredcwhite
Copy link
Member

This is looking good! I'll take it for a quick spin, but otherwise I expect to merge it in shortly. BTW, would it be possible to take "Tailwind" out of the list of example frameworks in the Sass section and move it to the PostCSS section, and then link to this installation guide? I'll try it out and make sure it works with the new PostCSS config so people can get set up right away.

@ayushn21
Copy link
Member Author

ayushn21 commented Dec 8, 2020

Sure, I've moved it now. Let me know if you'd like to alter the wording at all.

Going to try installing it myself as well to make sure it works :)

@ayushn21
Copy link
Member Author

ayushn21 commented Dec 8, 2020

I just tried installing Tailwind and it seems to work fine :D

I made a little change to the format of the PostCSS config so it matches up to what Tailwind have on their website so hopefully shouldn't be any confusion. Let me know if you run into any issues :)

@ayushn21 ayushn21 force-pushed the change-default-css-processor-to-postcss branch from 3ec2487 to 4f741d0 Compare December 8, 2020 20:41
@jaredcwhite
Copy link
Member

I would probably advocate for adding https://github.com/postcss/postcss-nested as a default here…I can't imagine ever wanting to write raw CSS without nesting, and it's easy for people to remove that plugin if they want but not obvious how they might add it in the first place.

@ayushn21
Copy link
Member Author

I get what you mean and I agree it's highly unlikely that anyone would want to write raw CSS without nesting; but if we're pitching the default PostCSS config as "empty", then I'm not entirely sure it's a good thing to put in opinions on how people will write their CSS.

For example, I do use nesting while writing CSS but I use a SASS plugin so I don't need a separate nesting plugin. I've never used TailwindCSS but as far as I'm aware, I don't think Tailwind users would need something like nesting either. Please do correct me if I'm wrong there.

Anyway the point I'm trying to make is if we put in the nesting plugin, there's a good case include other plugins like postcss-import as well.

So the question for me is do we want to position the PostCSS config as "empty", or do we want it to have an opinion on how to write CSS? If it's the latter than there's a good case to be made for a few more plugins to be included by default I reckon.

I also didn't include the CSS "autoloading" using require.context as I had described in my blog post because I was trying to keep the configuration as empty as possible.

I'm happy to proceed on your preference here. I don't have a particularly strong opinion either way.

@ayushn21
Copy link
Member Author

One other option could also be to leave the configuration empty, but have a dedicated documentation page for "PostCSS plugins recommended by Bridgetown" or something like that.

That could provide some guidance to newcomers and still leave the default config as basic as possible.

@jaredcwhite
Copy link
Member

One other option could also be to leave the configuration empty, but have a dedicated documentation page for "PostCSS plugins recommended by Bridgetown" or something like that.

That could provide some guidance to newcomers and still leave the default config as basic as possible.

All right, you convinced me. It's tricky to get the balance right between smart defaults and wide-open starting points, but if we start lean yet provide step-by-step instructions on how to add a few extra bits that might be pretty popular, that sounds good. It shouldn't hold up this PR, we can add additional documentation as a follow-up.

@ayushn21
Copy link
Member Author

Cool, sounds good to me!

Looking forward to the live stream tomorrow and keeping everything crossed that stuff doesn't catch fire 😬😄

@jaredcwhite jaredcwhite merged commit 0fb1aec into bridgetownrb:main Dec 11, 2020
@ayushn21 ayushn21 deleted the change-default-css-processor-to-postcss branch December 11, 2020 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Add Webpack example that uses PostCSS instead of Sass
2 participants