Do not emit @keyframes in @theme reference#16120
Merged
RobinMalfait merged 5 commits intomainfrom Jan 31, 2025
Merged
Conversation
Comment on lines
+457
to
+462
| // Do not track/emit `@keyframes`, if they are part of a `@theme reference`. | ||
| if (themeOptions & ThemeOptions.REFERENCE) { | ||
| replaceWith([]) | ||
| return WalkAction.Skip | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Hmm my initial thought was that we might want this in the Theme just like the --animate-* variables and just also add the themeOption for bookkeeping but we don't really do any validations for that anymore afterwards so if this works, it should be fine too 👍
Member
Author
There was a problem hiding this comment.
Yeah this just never really tracks it in Theme. It does mean that animate-foo won't emit the @keyframes, but that sounds correct to me because the idea is that this can be used in <style> blocks, assuming the @keyframes exist somewhere else already.
Just looking into why the other integration test fails. 😅
11696c9 to
1d86bd1
Compare
fb3435d to
177b28e
Compare
philipp-spiess
approved these changes
Jan 31, 2025
RobinMalfait
added a commit
that referenced
this pull request
Jan 31, 2025
This PR is an optimization where it will not emit empty rules and at-rules. I noticed this while working on #16120 where we emitted: ```css :root, :host { } ``` There are some exceptions for "empty" at-rules, such as: ```css @charset "UTF-8"; @layer foo, bar, baz; @Custom-Media --modern (color), (hover); @namespace "http://www.w3.org/1999/xhtml"; ``` These don't have a body, but they still have a purpose and therefore they will be emitted. However, if you look at this: ```css /* Empty rule */ .foo { } /* Empty rule, with nesting */ .foo { .bar { } .baz { } } /* Empty rule, with special case '&' rules */ .foo { & { &:hover { } &:focus { } } } /* Empty at-rule */ @media (min-width: 768px) { } /* Empty at-rule with nesting*/ @media (min-width: 768px) { .foo { } @media (min-width: 1024px) { .bar { } } } ``` None of these will be emitted. --------- Co-authored-by: Jordan Pittman <jordan@cryptica.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes na issue where
@keyframeswere emitted if they wre in a@theme referenceand anothe@theme {}(that is not a reference) was present.E.g.:
Produces:
With this PR, the produced CSS looks like this instead:
Note: the empty
:root, :hostwill be solved in a subsequent PR.Test plan
Added some unit tests, and a dedicated integration test.