-
Notifications
You must be signed in to change notification settings - Fork 186
Prep docs and post-install message for final v4.0.0 release #485
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
Conversation
I think 787bf26 left these instructions split across sections, I've tried to clean that up.
e5c7e79
to
976ddab
Compare
README.md
Outdated
We'll try to improve the upgrade process over time, but for now you may need to do some manual work to upgrade. | ||
### Updating CSS class names for v4 | ||
Before running the upgrade task, go to ``config/tailwind.config.js`` update the ``content`` part to: | ||
With some additional manual work the upstream upgrade tool will update your application's CSS class names to v4 conventions. **This is an optional step that requires a Javascript toolchain!** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think from this line up to line 198 should be part of the main Upgrade steps, since the upgrade rake task runs the upgrade tool, the tool will fail if the user has not installed the packages referenced in the config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These steps are necessary not only to migrate the class names but also to convert the configuration to the new CSS format, such as plugins and user custom configurations that might reference to tailwind default theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstream upgrade tool only fails if additional packages have been added and can't be found. A vanilla rails app doesn't need these steps, and they are complex and frankly hard to visually parse and follow as currently written.
I think if this is truly "required" then we should automate it. And if it's not worth automating we should keep it marked as optional. And if it's hard to automate then we need to file issues upstream to have the Tailwind project make the tool work better.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like, I'm really having a hard time understanding why these steps are necessary, and I'm sorry about that. The rake task works fine for me on my personal projects. And I understand that more complex setups need more complex steps, but then I think it's OK to leave those steps as a separate section. If you want to suggest alternative wording for the section maybe that would split the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes senses. Another suggestion is to move up the following excerpt and place it below Upgrade Steps title, maybe something like this:
Warning
In setups without JavaScript tooling, the update process may fail to fully migrate tailwind.config.js
because the tool assumes that the imported packages (e.g., tailwind plugins) are installed via a package manager, allowing them to be called. In this case, you should try following the instructions in Updating CSS class names for v4 which will install the needed javascript packages for the updater.
So the reader is aware upfront and doesn't have to go all the way down the guide to find a solution if something goes wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, will update the PR. Thank you!! 🙏❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!!
Side note: I sent you an email. You might want to check it when you have a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR with some more README changes. LMK what you think if you get a moment, otherwise I'll merge over the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks perfect to me.
Co-authored-by: Eric Gusmao <ericdegusmaopino@gmail.com>
@EricGusmao please take a look when you get a moment. 🙏