-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Ignore --tw-
variables during internal signature computation
#19156
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
10a32fc
to
41abcb1
Compare
@RobinMalfait Is this an issue of utilities like the Also being nitpicky: The PR title is a bit confusing. It could help if you prefix it with "Signature Computation: …" |
--tw-
variables--tw-
variables during internal signature computation
Updated the title 👍 It is also an issue with .backdrop-blur-sm {
--tw-backdrop-blur: blur(var(--blur-sm, 8px));
-webkit-backdrop-filter: var(--tw-backdrop-blur,) var(--tw-backdrop-brightness,) var(--tw-backdrop-contrast,) var(--tw-backdrop-grayscale,) var(--tw-backdrop-hue-rotate,) var(--tw-backdrop-invert,) var(--tw-backdrop-opacity,) var(--tw-backdrop-saturate,) var(--tw-backdrop-sepia,);
backdrop-filter: var(--tw-backdrop-blur,) var(--tw-backdrop-brightness,) var(--tw-backdrop-contrast,) var(--tw-backdrop-grayscale,) var(--tw-backdrop-hue-rotate,) var(--tw-backdrop-invert,) var(--tw-backdrop-opacity,) var(--tw-backdrop-saturate,) var(--tw-backdrop-sepia,);
} I also don't think it's super realistic to to try and migrate If we do want something like that, then I think that should be a separate PR 🤔 We also have the issue where |
@RobinMalfait What I'm thinking is if we remove
|
We're only removing So the final signature looks like this: /* blur */
.x {
--tw-blur: blur(8px);
filter: var(--tw-blur,)_var(--tw-brightness,)_var(--tw-contrast,)_var(--tw-grayscale,)_var(--tw-hue-rotate,)_var(--tw-invert,)_var(--tw-saturate,)_var(--tw-sepia,)_var(--tw-drop-shadow,);
}
/* contrast-100 */
.x {
--tw-contrast: contrast(100%);
filter: var(--tw-blur,)_var(--tw-brightness,)_var(--tw-contrast,)_var(--tw-grayscale,)_var(--tw-hue-rotate,)_var(--tw-invert,)_var(--tw-saturate,)_var(--tw-sepia,)_var(--tw-drop-shadow,);
} |
We already constant fold declarations, including normalization of `0.00%` to just `0%`. So we don't need this code anymore.
This will help with `[font-weight:400]` to `font-normal`, even though `font-normal` is actually implemented as: ```css .font-normal { --tw-font-weight: 400; font-weight: 400; } ```
Not super keen on this exception here, but it's something we do. Another solution here is to use `--tw-line-height` instead so it properly maps to the other property.
This way we don't have to hardcode the edge cases, but we have to make sure that the same value exists _somewhere_. We also make sure that the other property is also not a CSS variable. Otherwise we would run into: ```css .foo { --tw-a: 1; --tw-b: 1; --tw-c: 1; } ``` Because this would delete all the declarations
e8a6120
to
dc60f7a
Compare
This PR improves some of the signature computation logic. Right now, when you want to convert
[font-weight:400]
tofont-normal
it's not going to work because the signatures don't like up:So this PR essentially ignores
--tw-{property}
if the{property}
exists with the exact same value.The main reason we have this, is to make composition of utilities easier.
As with any of these upgrades, they are the expected behavior in most cases, but there could always be a case where you don't want this, but that's why the upgrade tool requires you to review each change before applying it. Intellisense will recommend the simplified class, but it's up to you to decide if you want to apply it or not.
There is a known edge case for
leading-{num}
, because the property isline-height
and the CSS variable is--tw-leading
. Right now we map--tw-leading
toline-height
but we can also update the leading classes to use--tw-line-height
instead but that feels like a bigger breaking change if people rely on these internal CSS variables...