-
Notifications
You must be signed in to change notification settings - Fork 180
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
feat: adding support for tailwind 4 #586
base: main
Are you sure you want to change the base?
Conversation
@@ -107,6 +106,10 @@ module.exports = { | |||
" | |||
/> | |||
|
|||
<p class="${hlmP}">If you are using Tailwind 4 add the following import to your global stylesheet:</p> | |||
|
|||
<spartan-code class="mt-4" code="@import '@spartan-ng/brain/hlm-tailwind-preset.css';" /> |
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.
That was fast! Such a clean solution :)
I was thinking we could generate this CSS instead - and only if it doesn't exist already - just like we do for our components. That way, if someone strips out hlm styling in favor of their custom styling, we don't need to download this style file along with the brain package.
We could totally look into this with a different PR, if at all.
This also came to my mind only because of general sentiment/feedback (frankly, it's just one comment) around users not likely needing to install the brain package either, so having to install it just for some CSS might not go down well. That is an entirely different debate, and I personally like having code centralized in one package. We could maybe even generator the entire brain package locally instead of installing, but that's irrelevant for this PR.
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.
Thanks, yes, so there are multiple approaches that could be take here, and I was deliberating between them so thoughts are welcome.
This approach works well for people who are potentially updating to tailwind 4, as the existing variables remain and the names do not change.
We could take the alternative approach which would be to add the Tailwind theme directly to the global stylesheet, the challenges here are:
- If we generate the @theme block in the stylesheet directly, great for new apps, but users who want to upgrade need to do more rework. I guess we could perhaps create a generator to migrate this, but that could be challenging as someone may have custom values, or multiple themes etc...
- Do we support both approaches? In that case our generators become more complex supporting multiple approaches based on a users setup.
Any thoughts are welcome, maybe its fine not to worry about the upgrade case 🤷♂️
This also came to my mind only because of general sentiment/feedback (frankly, it's just one comment) around users not likely needing to install the brain package either, so having to install it just for some CSS might not go down well. That is an entirely different debate, and I personally like having code centralized in one package. We could maybe even generator the entire brain package locally instead of installing, but that's irrelevant for this PR.
This is interesting, im not sure how much value there is in spartan without the brain package? That being said, I think the whole setup procedure could be greatly simplified, perhaps just a single ng add
command to set everything up rather than running multiple steps including some manual. This is something I might look into.
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 this PR supports the users well in terms of upgrading.
I don't think that that hlm works without brain, we decided that helm without brain is not really valuable.
This made some implementations a lot easier and we only heard about users that use brain without hlm but not the other way around.
I would say, for the ng add
command you mentioned we can think of adding the theme directly.
If an existing user wants to add the styles globally he can still do this. ;)
PR Checklist
Please check if your PR fulfills the following requirements:
guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
PR Type
What kind of change does this PR introduce?
Which package are you modifying?
What is the current behavior?
N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Adding support for Tailwind 4. This adds tailwind version detection to the theme generator and adds the preset import automatically if Tailwind 4 is detected.