-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Address follow-up work for #14639 #14650
Conversation
894ddd8
to
336f509
Compare
336f509
to
c73242b
Compare
) | ||
|
||
test( | ||
'does not upgrade JS config files with typography styles in the theme config', |
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.
Do we specifically check for typography or do we bail on any complex object under 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.
This bails out right now because it's using a function inside 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.
Got it so should that be how the test is named then since it's not really about typography stuff? It's just testing another heuristic right.
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.
Also do we have a heuristic that prevents deeply nested objects as theme values?
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.
Also do we have a heuristic that prevents deeply nested objects as theme values?
I don't think we do, but this is what this should eventually be, I think. I think in the compat layer right now these typography values would be added to the Theme
, I did not see any heuristic trying to remove it.
We'll need to be able to support functions in the theme object and when we do we have to figure out how to still not add stuff for typography. That's really the two differences here.
Right now that test is not different from the one above "does not upgrade JS config files with functions in the theme config", so we can drop it. I just wanted to be explicit about this case since when we add support for the one above, we probably don't want to forget that this should not do weirdly nested stuff now.
Since this PR isn't merged yet, one thing I noticed is that if you use the default config most people have (from https://tailwindcss.com/docs/installation), that looks something like: /** @type {import('tailwindcss').Config} */
module.exports = {
content: ["./src/**/*.{html,js}"],
theme: {
extend: {},
},
plugins: [],
} Then we inject a @import "tailwindcss";
@source "./**/*.{html,js}";
@theme {} In this case we can prevent injecting the |
@thecrypticace Thanks for the added heuristics! I have updated the tests a bit so they now go off real world data we want to cover (the typography extension use case) but the heuristics is still sound 👍 |
seems good to me now — probably want to get @adamwathan to take a look before merging |
966addb
to
7daed84
Compare
This PR adds a few more test cases to #14639 and updates the documentation.