-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
feat: allow using @variant with multiple comma-separated variants #19526
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
base: main
Are you sure you want to change the base?
feat: allow using @variant with multiple comma-separated variants #19526
Conversation
WalkthroughThe pull request adds support for comma-separated variants inside 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tailwindcss/src/variants.ts (1)
1215-1231: Consider cloning nodes for each variant iteration to prevent potential shared-state issues.Each iteration creates a new
styleRulewith the samevariantNode.nodesreference. While variant functions typically reassignr.nodesrather than mutating in place, the underlying AST nodes (e.g., declaration objects) are shared across iterations. If any downstream processing mutates these objects in place, all variants would be affected.For defensive coding, consider cloning the nodes for each iteration using the already-imported
cloneAstNode:🔎 Suggested defensive change
for (let variant of variants) { // Starting with the `&` rule node - let node = styleRule('&', variantNode.nodes) + let node = styleRule('&', variantNode.nodes.map(cloneAstNode))
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/tailwindcss/src/index.test.tspackages/tailwindcss/src/variants.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/index.test.ts (2)
packages/tailwindcss/src/test-utils/run.ts (1)
compileCss(4-11)integrations/utils.ts (1)
css(519-519)
🔇 Additional comments (1)
packages/tailwindcss/src/index.test.ts (1)
5302-5332: Test coverage looks good for the primary use case.The test correctly verifies that comma-separated variants produce independent rule sets with proper variant-specific behavior (hover wrapped in
@media (hover: hover), focus applied directly).Consider adding edge case tests in a follow-up for robustness:
- Three or more variants:
@variant hover, focus, active { ... }- Whitespace variations:
@variant hover,focus { ... }vs@variant hover , focus { ... }- Nested comma-separated variants
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tailwindcss/src/index.test.ts (1)
5337-5509: Excellent test coverage for comma-separated variant functionality!The new test suite comprehensively validates the comma-separated
@variantfeature with tests covering basic usage, multiple variants, whitespace handling, and nesting. The test structure is consistent with existing patterns and the snapshots confirm correct behavior.Optional enhancement: Consider adding edge case tests for invalid inputs to ensure graceful handling:
Suggested additional test cases
it('should handle trailing commas gracefully', async () => { await expect( compileCss( css` .btn { background: black; @variant hover, focus, { background: red; } } @tailwind utilities; `, [], ), ).resolves.toBeDefined() // or .rejects if it should error }) it('should handle empty variant names', async () => { await expect( compileCss( css` .btn { @variant hover, , focus { background: red; } } `, [], ), ).resolves.toBeDefined() // or .rejects if it should error })These additional tests would help ensure the parser handles malformed input predictably, but the current coverage is sufficient for the main feature.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tailwindcss/src/index.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/index.test.ts (2)
packages/tailwindcss/src/test-utils/run.ts (1)
compileCss(4-11)integrations/utils.ts (1)
css(519-519)
discussion: #19319
Summary
in css, now can apply the same styles to multiple variants in one place. e.g.
Test plan
@variantwith multiple, comma-separated variants