feat(css-serializer): support custom properties in keyframes#2429
Conversation
🦋 Changeset detectedLatest commit: b60fc8b The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a helper to extract custom properties from AST blocks and wired it into parsing for StyleRule and KeyframesRule; KeyframesStyle now includes a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tools/css-serializer/src/parse.ts`:
- Around line 262-263: transformBlock currently strips custom properties (--*)
so keyframe declarations captured by transformVariables are never serialized;
update the keyframes encoding path to include those variables when emitting
keyframes by merging transformVariables(rule.block) into the serialized style
for keyframes (or adjust the encoder to iterate both keyframesStyle.style and
keyframesStyle.variables). Specifically, where keyframes are built from
keyframesStyle.style, also incorporate keyframesStyle.variables (the output of
transformVariables) so custom properties like --bg-color are preserved for
functions handling serialization; reference transformBlock, transformVariables,
and keyframesStyle.style in your changes.
In `@packages/tools/css-serializer/test/__preparation__/keyframes.css`:
- Around line 37-50: The keyframes fixture fails stylelint: rename the keyframe
identifier testKeyframeDeclaration to a name matching the project's
keyframes-name-pattern (e.g., test-keyframe-declaration) and add the required
empty lines before declarations to satisfy declaration-empty-line-before (ensure
there is a blank line after each opening brace of the keyframe percentage blocks
and before the first declaration, e.g., before --bg-color in the 0%, 50%, and
100% blocks); update the `@keyframes` name in the declaration and any references
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d29950a-8472-4a47-9b39-4d52baf2e872
⛔ Files ignored due to path filters (1)
packages/tools/css-serializer/test/__snapshots__/lynx.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/tools/css-serializer/src/parse.tspackages/tools/css-serializer/src/types/LynxStyleNode.tspackages/tools/css-serializer/test/__preparation__/keyframes.css
packages/tools/css-serializer/test/__preparation__/keyframes.css
Outdated
Show resolved
Hide resolved
Merging this PR will improve performance by 17.18%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#8719 Bundle Size — 730.24KiB (0%).b60fc8b(current) vs b630df2 main#8717(baseline) Bundle metrics
|
| Current #8719 |
Baseline #8717 |
|
|---|---|---|
 Initial JS |
43.63KiB |
43.63KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
 Cache Invalidation |
0% |
48.73% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
147(-1.34%) |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
34.69% |
34.69% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8719 |
Baseline #8717 |
|
|---|---|---|
| Other |
385.55KiB |
385.55KiB |
| JS |
342.53KiB |
342.53KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch usczz:p/zhouzhitao/feature/suppo... Project dashboard
Generated by RelativeCI Documentation Report issue
React External#263 Bundle Size — 590.57KiB (0%).b60fc8b(current) vs b630df2 main#261(baseline) Bundle metrics
|
| Current #263 |
Baseline #261 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch usczz:p/zhouzhitao/feature/suppo... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7144 Bundle Size — 236.79KiB (0%).b60fc8b(current) vs b630df2 main#7142(baseline) Bundle metrics
|
| Current #7144 |
Baseline #7142 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
70 |
70 |
| Duplicate Code |
46.11% |
46.11% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7144 |
Baseline #7142 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.03KiB |
91.03KiB |
Bundle analysis report Branch usczz:p/zhouzhitao/feature/suppo... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#277 Bundle Size — 206.05KiB (0%).b60fc8b(current) vs b630df2 main#275(baseline) Bundle metrics
|
| Current #277 |
Baseline #275 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
45.77% |
45.77% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #277 |
Baseline #275 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
94.81KiB |
94.81KiB |
Bundle analysis report Branch usczz:p/zhouzhitao/feature/suppo... Project dashboard
Generated by RelativeCI Documentation Report issue
usczz
force-pushed
the
p/zhouzhitao/feature/support_custom_property_keyframe
branch
2 times, most recently
from
fe8b3b9 to
20bdd5c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tools/css-serializer/test/__preparation__/keyframes.css (1)
37-52: Good test coverage for keyframe CSS variables.This fixture properly exercises the new
transformVariablesfunctionality for keyframes, testing custom property declarations alongsidevar()usage across multiple keyframe blocks.Minor: Stylelint flags missing empty lines before
background-colordeclarations (lines 40, 45, 50). If the project enforces this rule for test fixtures, consider adding blank lines:Optional formatting fix
`@keyframes` test-keyframe-declaration { 0% { --bg-color: red; + background-color: var(--bg-color); } 50% { --bg-color: green; + background-color: var(--bg-color); } 100% { --bg-color: blue; + background-color: var(--bg-color); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tools/css-serializer/test/__preparation__/keyframes.css` around lines 37 - 52, Test fixture keyframes lack blank lines before the background-color declarations, which Stylelint flags; update the `@keyframes` block named test-keyframe-declaration so each keyframe rule (0%, 50%, 100%) has an empty line between the custom property declaration (--bg-color: ...) and the background-color: var(--bg-color); line to satisfy the project's formatting rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tools/css-serializer/test/__preparation__/keyframes.css`:
- Around line 37-52: Test fixture keyframes lack blank lines before the
background-color declarations, which Stylelint flags; update the `@keyframes`
block named test-keyframe-declaration so each keyframe rule (0%, 50%, 100%) has
an empty line between the custom property declaration (--bg-color: ...) and the
background-color: var(--bg-color); line to satisfy the project's formatting
rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d6aa598-bfd8-472e-b2bc-958046e897e6
⛔ Files ignored due to path filters (1)
packages/tools/css-serializer/test/__snapshots__/lynx.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/tools/css-serializer/src/parse.tspackages/tools/css-serializer/src/types/LynxStyleNode.tspackages/tools/css-serializer/test/__preparation__/keyframes.css
✅ Files skipped from review due to trivial changes (1)
- packages/tools/css-serializer/src/types/LynxStyleNode.ts
usczz
force-pushed
the
p/zhouzhitao/feature/support_custom_property_keyframe
branch
from
20bdd5c to
86782a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/tools/css-serializer/test/__preparation__/keyframes.css (1)
37-52:⚠️ Potential issue | 🟡 MinorGood test coverage, but fix stylelint violations for
declaration-empty-line-before.The fixture correctly tests custom properties in keyframes. The kebab-case naming is compliant, but stylelint flags missing empty lines before
background-colordeclarations at lines 40, 45, and 50.Proposed fix to satisfy stylelint
`@keyframes` test-keyframe-declaration { 0% { --bg-color: red; + background-color: var(--bg-color); } 50% { --bg-color: green; + background-color: var(--bg-color); } 100% { --bg-color: blue; + background-color: var(--bg-color); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tools/css-serializer/test/__preparation__/keyframes.css` around lines 37 - 52, The stylelint rule declaration-empty-line-before is violated for the background-color declarations inside `@keyframes` test-keyframe-declaration; fix it by adding a blank line immediately before each background-color declaration in the 0%, 50%, and 100% keyframe blocks so that the lines with background-color follow an empty line after the custom property (--bg-color) assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/tools/css-serializer/test/__preparation__/keyframes.css`:
- Around line 37-52: The stylelint rule declaration-empty-line-before is
violated for the background-color declarations inside `@keyframes`
test-keyframe-declaration; fix it by adding a blank line immediately before each
background-color declaration in the 0%, 50%, and 100% keyframe blocks so that
the lines with background-color follow an empty line after the custom property
(--bg-color) assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d38daabe-c8cb-4bdc-a663-631a23c3e2ac
⛔ Files ignored due to path filters (1)
packages/tools/css-serializer/test/__snapshots__/lynx.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/tools/css-serializer/src/parse.tspackages/tools/css-serializer/src/types/LynxStyleNode.tspackages/tools/css-serializer/test/__preparation__/keyframes.csspackages/web-platform/web-core/ts/encode/encodeCSS.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tools/css-serializer/src/types/LynxStyleNode.ts
usczz
force-pushed
the
p/zhouzhitao/feature/support_custom_property_keyframe
branch
from
86782a7 to
4aedb0e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/tools/css-serializer/test/__preparation__/keyframes.css (1)
38-50:⚠️ Potential issue | 🟡 MinorAdd required empty lines before
background-colordeclarations.
declaration-empty-line-beforeis still failing at Line 40, Line 45, and Line 50. Please keep a blank line before eachbackground-colordeclaration in these keyframe blocks.Suggested fix
`@keyframes` test-keyframe-declaration { 0% { --bg-color: red; + background-color: var(--bg-color); } 50% { --bg-color: green; + background-color: var(--bg-color); } 100% { --bg-color: blue; + background-color: var(--bg-color); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tools/css-serializer/test/__preparation__/keyframes.css` around lines 38 - 50, Add a blank line before each "background-color" declaration inside the keyframe blocks (the 0%, 50%, and 100% rules) so they comply with the declaration-empty-line-before rule; locate the background-color lines within the keyframe selectors (0%, 50%, 100%) and insert an empty line immediately above each "background-color: var(--bg-color);" declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/tools/css-serializer/test/__preparation__/keyframes.css`:
- Around line 38-50: Add a blank line before each "background-color" declaration
inside the keyframe blocks (the 0%, 50%, and 100% rules) so they comply with the
declaration-empty-line-before rule; locate the background-color lines within the
keyframe selectors (0%, 50%, 100%) and insert an empty line immediately above
each "background-color: var(--bg-color);" declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea57a410-fc66-48dc-9d0a-ffa2f599d608
⛔ Files ignored due to path filters (2)
packages/tools/css-serializer/test/__snapshots__/lynx.spec.ts.snapis excluded by!**/*.snappackages/web-platform/web-core/tests/__snapshots__/encode.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
packages/tools/css-serializer/src/parse.tspackages/tools/css-serializer/src/types/LynxStyleNode.tspackages/tools/css-serializer/test/__preparation__/keyframes.csspackages/web-platform/web-core/tests/encode.spec.tspackages/web-platform/web-core/ts/encode/encodeCSS.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tools/css-serializer/src/types/LynxStyleNode.ts
- packages/tools/css-serializer/src/parse.ts
- packages/web-platform/web-core/tests/encode.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web-platform/web-core/ts/encode/encodeCSS.ts
usczz
force-pushed
the
p/zhouzhitao/feature/support_custom_property_keyframe
branch
2 times, most recently
from
61863be to
602dcf4
Compare
|
@Sherry-hue PTAL |
usczz
force-pushed
the
p/zhouzhitao/feature/support_custom_property_keyframe
branch
2 times, most recently
from
c53d761 to
6bdaf24
Compare
- Extract `transformVariables` utility function in `parse.ts`. - Add `variables` field to `KeyframesStyle` type to store parsed custom properties. - Collect CSS variables for each keyframe block in `@keyframes` rules. - Add test cases for `@keyframes` with CSS variables.
usczz
force-pushed
the
p/zhouzhitao/feature/support_custom_property_keyframe
branch
from
6bdaf24 to
b60fc8b
Compare
4 participants
transformVariablesutility function inparse.ts.variablesfield toKeyframesStyletype to store parsed custom properties.@keyframesrules.@keyframeswith CSS variables.Summary by CodeRabbit
Checklist