-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Do not emit @keyframes in @theme reference
#16120
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
Do not emit @keyframes in @theme reference
#16120
Conversation
| // Do not track/emit `@keyframes`, if they are part of a `@theme reference`. | ||
| if (themeOptions & ThemeOptions.REFERENCE) { | ||
| replaceWith([]) | ||
| return WalkAction.Skip | ||
| } | ||
|
|
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 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 👍
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 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
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 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.