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

Look up tailwind.config.js anywhere in the app directory (except dirs ignored explicitly) #204

Merged
merged 8 commits into from
Sep 6, 2024

Conversation

trinitytakei
Copy link
Contributor

@trinitytakei trinitytakei commented Jun 24, 2024

The Problem

As discussed here: https://discord.com/channels/1082611227827638303/1254781033791098940 , tailwind.config.js can show up in several places in a Rails app, based on how/when you installed Tailwind.

However, the Phlex install generator currently assumes that tailwind.config.js should always be in the Rails.root directory. This can lead to some surprises if the user installing Phlex has the file somewhere else (config/tailwind.config.js if installed with the tailwind-rails generator, or app/javascript/stylesheets/tailwind.config.js for older (I'm guessing webpacker) apps; possibly other places, depending on your JS bundler config and whatnot).

The Proposed Solution

This PR aims to rectify the situation by looking up the location of tailwind.config.js. Except some directories explicitly ignored by the generator (like node_modules, vendor and such), it will look fortailwind.config.js anywhere in the app directory.

EDIT after @joeldrapper's review: It's cleaner and more maintainable to go with the whitelist approach ("look here: ... ") than the blacklist one ("look anywhere BUT here: ..."). Just keep in mind that your tailwind.config.js config has to be in either

  • Rails.root
  • app/**
  • config/**

🤔

I'm assuming the previous implementation semantic was something like "If tailwind.config.js is not in Rails.root, this application is not using Tailwind, thus there's no need to apply any changes to tailwind.config.js, kthxbye".

The current one is along the lines of "If you are using Tailwind, and you are not doing something extremely crazy (like having multiple tailwind.config.js files or storing them outside of the sensible directories (Rails.root, app/**, config/**), we'll find the file and apply the modifications".

Comment on lines 7 to 23
IGNORED_TAILWIND_CONFIGURATION_LOOKUP_DIRECTORIES = %w[
.git
.github
.ruby-lsp
bin
coverage
db
docs
log
node_modules
public
spec
storage
test
tmp
vendor
].freeze
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t like the idea of Phlex maintaining an ignore list that users can’t update. Instead, could we look in just app/** and config/**, ignoring everything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally! See updated PR.

could we look in just app/** and config/**

I also added "#{Rails.root}/tailwind.config.js" for good measure 😅

@joeldrapper
Copy link
Collaborator

Thanks for opening this and sorry it took so long to review.

@trinitytakei trinitytakei force-pushed the tailwind-config-js-lookup branch from 0e5b49d to 5d487c9 Compare August 14, 2024 10:11
@trinitytakei
Copy link
Contributor Author

No worries, summer is slow season over here as well!

@trinitytakei trinitytakei force-pushed the tailwind-config-js-lookup branch from 5d487c9 to e50ed04 Compare August 14, 2024 11:52
"#{Rails.root}/tailwind.config.js",
"#{Rails.root}/app/**/tailwind.config.js",
"#{Rails.root}/config/**/tailwind.config.js",
])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet another questionable Rubocop thingamajig 😵‍💫

Not sure how to handle these (now and in the future)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just do it the way you feel is right and we’ll adjust the Rubocop configuration accordingly.

@trinitytakei
Copy link
Contributor Author

@joeldrapper I believe this is good to go as well (at least Rubocop / bin/test are 🟢).

@joeldrapper joeldrapper merged commit 2715aae into phlex-ruby:main Sep 6, 2024
12 of 13 checks passed
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.

3 participants