-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Support specifying gradient interpolation method via modifier #14984
Conversation
// Invalid but proves not converted to `in oklch longer hue` when used | ||
// as an arbitrary value | ||
'bg-linear-to-r/[longer]', |
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.
We should move this one to the list below this one where we test that nothing is being generated to prevent accidentally updating snapshots with invalid code.
The one with .toEqual('')
on line 10671
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 does generate CSS though, it’s just not valid CSS but we never really validate arbitrary values in Tailwind. Couldn’t think of another way to test that “longer” isn’t expanded into “in oklch longer hue” when used as an arbitrary value.
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.
Hmm I see. When I put it on the second blob it didn't generate CSS but that's because we generate CSS, but Lightning CSS throws it out because it's invalid.
I think we have other examples that technically generate something but Lightning CSS throw it out.
7f0d796
to
5c39f33
Compare
if (!negative && linearGradientDirections.has(value)) { | ||
value = linearGradientDirections.get(value)! | ||
} else if (isPositiveInteger(value)) { | ||
value = negative ? `calc(${value}deg * -1)` : `${value}deg` |
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.
Is there actually a benefit of having that -1
calc in CSS instead of doing it on the JS value?
value = negative ? `calc(${value}deg * -1)` : `${value}deg` | |
value = negative ? `${Number(value) * -1}deg` : `${value}deg` |
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.
Yeah can change this! This wasn't introduced in this PR though.
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.
Yeah I know was just curious haha
let value = candidate.value.value | ||
return [ | ||
decl('--tw-gradient-position', `${value},`), | ||
decl('background-image', `conic-gradient(var(--tw-gradient-stops,${value}))`), |
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.
Should there also be a comma in the fallback?
decl('background-image', `conic-gradient(var(--tw-gradient-stops,${value}))`), | |
decl('background-image', `conic-gradient(var(--tw-gradient-stops,${value},))`), |
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.
There shouldn't be, the comma is only because --tw-gradient-stops
variable uses the --tw-gradient-position
variable internally, slightly simplified it looks like this:
--tw-gradient-stops: var(--tw-gradient-position) var(--tw-gradient-from) var(--tw-gradient-to);
…where "from" and "to" are the gradient colors. So the variable includes the "," so that the resolved value looks something like:
--tw-gradient-stops: from 45deg, red, blue;
But with conic gradients the "from 45deg" part is optional, so if someone doesn't set --tw-gradient-position
we want the resolved variable to be:
--tw-gradient-stops: red, blue;
…which is why it's important that the comma is part of --tw-gradient-position
because it needs to be absent if that variable isn't set.
It's not needed in the fallback because if we included it you'd get a resolved conic gradient with a trailing comma which would break it:
background-image: conic-gradient(from 45deg,);
The expectation is that if you use an arbitrary conic gradient and don't use the from-*
or to-*
utilities, you've provided a complete conic gradient value as the arbitrary value:
bg-conic-[from_45deg,red,blue]
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.
Ohh yes makes sense, I thought this was using the variable defined above (--tw-gradient-position
) 🤦
if (!candidate.value) { | ||
return [ | ||
decl('--tw-gradient-position', `${interpolationMethod},`), | ||
decl('background-image', `conic-gradient(var(--tw-gradient-stops))`), |
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 arbitrary value has a fallback here, should we add one here too (or rm the fallback above)?
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 is for the bg-conic
utility with no value, which just can't do anything at all unless you also use the from-*
and to-*
utilities, so since it's not possible for the utility to have any effect on its own it didn't make sense to me to include the fallback.
This is just junk CSS for example:
background-image: conic-gradient(in oklch);
…and that's what you'd be getting if the fallback triggered. Since it's invalid either way it's less code to just not include it, and also means we don't imply that it's supposed to work or anything.
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.
ah yes makes sense, I was confused there because I didn't read the variables used correctly...
let interpolationMethod = resolveInterpolationModifier(candidate.modifier) | ||
return [ | ||
decl('--tw-gradient-position', `${interpolationMethod},`), | ||
decl('background-image', `radial-gradient(var(--tw-gradient-stops))`), |
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 no fallback here so I guess we just rm the one on line 2396: https://github.com/tailwindlabs/tailwindcss/pull/14984/files?diff=split&w=1#diff-036c40df8e1498a5c399fb971238a19fcdab257001b9703661a46a110984b56aR2396
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.
Same thing as the conic one — values like radial-gradient(red, blue)
are valid so it's useful to just have a bg-radial
utility that specifies no position/direction but radial-gradient(in oklch)
isn't valid so the fallback isn't useful.
This PR adds support for specifying a color interpolation method for all gradient utilities using a modifier:
Supported bare values are any valid color space keyword, as well as the special keywords
shorter
,longer
,increasing
, anddecreasing
, which are shortcuts forin oklch {keyword} hue
.Arbitrary values are also supported and are used as-is, so the
in
keyword is not automatically included for you:Modifiers are not supported when using arbitrary values for the actual gradient, as it's expected that your arbitrary gradient value contain all of the details you want in your gradient:
Resolves #14955, but it may still be wise to make
oklab
the default since I do sort of agree with the poster there that most people probably expect a gradient between two colors to sort of just "fade" between them rather than interpolate between them around the color wheel.