Skip to content
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

Merged
merged 2 commits into from
Jan 9, 2025
Merged

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Jan 9, 2025

Closes #15438
Closes #15560
Closes #15561
Closes #15562

This PR upgrades lightningcss to 1.29.0 and uses the new feature flag to disable the light-dark function transpilation.

@philipp-spiess philipp-spiess marked this pull request as ready for review January 9, 2025 09:47
@philipp-spiess philipp-spiess requested a review from a team as a code owner January 9, 2025 09:47
@philipp-spiess philipp-spiess marked this pull request as draft January 9, 2025 10:03
@philipp-spiess philipp-spiess marked this pull request as ready for review January 9, 2025 11:02
Copy link
Member

@RobinMalfait RobinMalfait left a 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;
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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:
image

After this change, the styles would look like:
image

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 😄)

Copy link
Member

@RobinMalfait RobinMalfait Jan 9, 2025

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.

Copy link
Member Author

@philipp-spiess philipp-spiess Jan 9, 2025

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?

Comment on lines -6842 to -6845
--lightningcss-light: ;
--lightningcss-dark: initial;
--lightningcss-light: ;
--lightningcss-dark: initial;
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wew, finally!

@philipp-spiess philipp-spiess force-pushed the chore/upgrade-lightningcss branch from 5dd2f7b to 238062a Compare January 9, 2025 15:59
@philipp-spiess philipp-spiess merged commit a11c80d into next Jan 9, 2025
5 checks passed
@philipp-spiess philipp-spiess deleted the chore/upgrade-lightningcss branch January 9, 2025 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v4] light-dark is broken in optimized build
2 participants