-
Notifications
You must be signed in to change notification settings - Fork 187
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
doc: update README upgrade guide for css class names
I think 787bf26 left these instructions split across sections, I've tried to clean that up.
- Loading branch information
commit e37ed78f5aced5f8aac1a5176659c6e66734672b
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.