Skip to content

Allow postcss #222

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

Closed
wants to merge 8 commits into from
Closed

Allow postcss #222

wants to merge 8 commits into from

Conversation

muriloime
Copy link

This PR adds support to adding postcss argument to tailwind cli

@flavorjones
Copy link
Member

Hi, thanks for proposing these changes.

Big picture question: does this need to be something that the user has to opt-in to? If the postcss.config.js file is present would it be better to have this gem silently pass the --postcss option? Put another way: is there a reason users wouldn't want to pass --postcss if that file was present?

If the answer to the above is "this should be a user option" then there are some things going on in this PR that I'd like to ask you to change:

  • do not bump the version number, that's something we do during the release process and not in a feature or bugfix PR
  • please add test coverage for this command variation in test/lib/tailwindcss/commands_test.rb
  • please remove the "contributing" section from the readme, there's a separate CONTRIBUTING.md document you can add to if you like
  • please clean up the commit history

@muriloime
Copy link
Author

Hi @flavorjones thanks for your comments. I think it is a good idea, but it does not follow the principle of least surprise IMO. I can imagine people having old postcss files in their repos where this could break.

No strong opinion though, as this could be railsy "convention over configuration" way

If you agree I can make the proposed changes and update the PR.

cheers

@flavorjones
Copy link
Member

I've never used postcss files, so I don't have an opinion either way. Why would someone have an "old postcss file" and not want to use it? I'm asking questions in order to understand.

@muriloime
Copy link
Author

Of course, no worries.

I came from a webpacker/postcss rails installation (which was default a few years ago) and migrated to a nodeless environment in production with tailwindcss-rails and importmap. In my case I had deleted the postcss.config.js , but I can imagine folks who forgot to do this in the migration, and after a gem update that would break.

But all of this is speculative

@basiszwo
Copy link

Any chance of moving this forward? Can I help doing so?

@flavorjones
Copy link
Member

@muriloime As I mentioned above, I'm not very familiar with postcss, so I don't understand why a postcss.config.js file would exist if people didn't want to use it.

From the standpoint of "convention over configuration" it feels like we should detect its presence and pass the appropriate options automatically to tailwindcss ... but if you feel strongly that this would surprise people and the workaround isn't simply deleting the file, then I'll trust your judgement.

@basiszwo
Copy link

Regarding @flavorjones comment, I'd follow the convention over configuration paradigm. Old configurations will probably blow up anyway if there's a postcss configuration in place which shouldn't.

What I found the last days playing around with the postcss integration in a rails 7 project using importmaps is the following.
I got errors from postcss stating that it couldn't be found in the node_modules folder. I am going the default importmap route in my rails 7 project, thus not having npm / yarn by default.

To make postcss work I had to bring in yarn. This has to be considered when for the convention over configuration way.
In fact it's the configuration file (postcss.config.js) and the postcss package.

@muriloime how did you manage to have postcss in place? also using yarn?

@muriloime
Copy link
Author

Hi there, I've just made the proposed changes. Because the way this gems works ( producing cli versions for each platforma), I don't have an easy way to test it, other than publish a gem like https://rubygems.org/gems/tailwindcss-rails-alt , but I realized I lost my 2FA rubygems credentials.

@basiszwo I use heroku, and use a node buildpack and a very clean package.json to run postcss

@basiszwo
Copy link

basiszwo commented Feb 7, 2023

@muriloime for the README part. Isn't it that the postcss.config.js needs to require the already existing tailwind config at config/tailwind.config.js?

module.exports = {
  plugins: [
    require('tailwindcss')('./config/tailwind.config.js')
  ]
}

If so, maybe covering this aspect in the README would be very helpful.

Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

  • please add test coverage for this command variation in test/lib/tailwindcss/commands_test.rb
  • please remove the "contributing" section from the readme, there's a separate CONTRIBUTING.md document you can add to if you like
  • please clean up the commit history

@@ -147,6 +151,14 @@ The inline version also works:
<section class="bg-[url('image.svg')]">Has the image as it's background</section>
```

### Contributing
Copy link
Member

Choose a reason for hiding this comment

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

There is a CONTRIBUTING.md file which I think covers this topic. If not, please make changes to that file and not the user-facing README.

Copy link
Member

Choose a reason for hiding this comment

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

I've added content addressing how to develop against this gem in e22c8ce

@duduribeiro
Copy link
Contributor

hey @muriloime 👋

do you need help to get this PR to the mainline? If so I can try help you. Would be nice to have postcss supported on this gem

@flavorjones
Copy link
Member

closing in favor of #316

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.

4 participants