-
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
Upgrade lightningcss
to 1.29.0
#15576
Conversation
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.
Just a question, but apart from that looks good!
@@ -205,7 +205,7 @@ summary { | |||
ol, | |||
ul, | |||
menu { | |||
list-style: none; | |||
list-style-type: none; |
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 wonder if this is enough. The list-style: none;
expands to:
list-style-position: initial;
list-style-image: initial;
list-style-type: none;
So if another stylesheet sets the list-style-position
or list-style-image
to something else then this would be a breaking change technically.
However, I think our main goal was to use list-style-type: none;
in the first place to get rid of the visual dot/number.
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.
@RobinMalfait Not sure I understand, preflight is for normalizing user-agent styling and there initial
is the default anyways, right? Any other stylesheets that could change this are probably in a different layer/unlyaered so they'd take presedence or am I missing somehting?
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 just wanted to be 100% sure that this is the correct fix without causing other (subtle) issues since the computed styles would be different.
I think a lot of people will use @layer base
for their preflight resets as well. I was trying to come up with an example where this change would not be enough/cause issues. E.g.:
If the setup looks like this:
/* ./styles.css */
@import "tailwindcss";
@import "./my-base.css";
/* ./my-base.css */
@layer base {
ul {
list-style-position: inside;
}
}
If you then inspect a ul
on the page, before this change, the styles would look like:
After this change, the styles would look like:
So while there is a difference in the computed values (the list-style-image
is gone) I don't think there is an actual issue. I was trying to come up with an example where this wouldn't be the case but couldn't find any so far (which is good 😄)
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.
Actually, it's just the devtools that hide the list-style-image
, window.getComputedStyle()
does have a none
value for list-style-image
.
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 think it would only be an issue if the initial CSS looks like this, right?
/* ./styles.css */
@import "./my-base.css";
@import "tailwindcss";
i.e. the overwrite comes before our resets. However, that also meant you had CSS in there before that was overwritten anyways so chances are that they'd have reordered this already otherwise this has had no effects I think?
--lightningcss-light: ; | ||
--lightningcss-dark: initial; | ||
--lightningcss-light: ; | ||
--lightningcss-dark: initial; |
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.
😍
@@ -14376,7 +14349,7 @@ test('transition', async () => { | |||
} | |||
|
|||
.transition { | |||
transition-property: color, background-color, border-color, text-decoration-color, fill, stroke, --tw-gradient-from, --tw-gradient-via, --tw-gradient-to, opacity, box-shadow, transform, translate, scale, rotate, filter, -webkit-backdrop-filter, -webkit-backdrop-filter, -webkit-backdrop-filter, backdrop-filter; | |||
transition-property: color, background-color, border-color, text-decoration-color, fill, stroke, --tw-gradient-from, --tw-gradient-via, --tw-gradient-to, opacity, box-shadow, transform, translate, scale, rotate, filter, -webkit-backdrop-filter, backdrop-filter; |
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.
Wew, finally!
5dd2f7b
to
238062a
Compare
Closes #15438
Closes #15560
Closes #15561
Closes #15562
This PR upgrades
lightningcss
to1.29.0
and uses the new feature flag to disable the light-dark function transpilation.