fix(theme): keep variants when replacing slot classes in app config - #6824
fix(theme): keep variants when replacing slot classes in app config#6824benjamincanac wants to merge 1 commit into
Conversation
Merging this PR will improve performance by 17.54%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | button (~6 slots) |
446 µs | 379.5 µs | +17.54% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/config-slot-replacer-variants (aab7edd) with v4 (795c353)
commit: |
📝 WalkthroughWalkthroughThe change separates construction-time replacers from call-time replacers. Configuration functions now resolve theme classes before Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/runtime/types/tv.tsParsing error: Unexpected token { src/runtime/utils/tv.tsParsing error: Unexpected token { test/utils/tv.spec.tsParsing error: Unexpected token as 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/types/tv.ts`:
- Around line 5-9: The call-time replacer documentation must clarify that it
replaces resolved classes, while accompanying plain classes still merge into the
result. Update src/runtime/types/tv.ts lines 5-9 and
docs/content/docs/1.getting-started/5.theme/3.components.md lines 406-408,
replacing the claim that returned classes are the only ones left with this
qualification.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 499cceaa-fb88-4f93-ba2b-785196a025ad
📒 Files selected for processing (5)
docs/content/docs/1.getting-started/5.theme/3.components.mdskills/nuxt-ui/references/guidelines/design-system.mdsrc/runtime/types/tv.tssrc/runtime/utils/tv.tstest/utils/tv.spec.ts
| * instead of merging onto them, returning the classes to use in their place. | ||
| * In `app.config.ui` it receives the slot's own theme classes and replaces only | ||
| * those — `variants` and `compoundVariants` still apply on top. In `:ui` / `class` | ||
| * it runs after variant resolution, so it receives the fully resolved class | ||
| * string and its return value is all that remains on the slot. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document plain classes supplied with a call-time replacer.
A call-time replacer removes the resolved class chain. It does not remove plain values supplied alongside the replacer. For example, [() => 'text-xl', 'opacity-50'] resolves to both classes.
src/runtime/types/tv.ts#L5-L9: state that the replacer replaces resolved classes, while accompanying plain classes still merge.docs/content/docs/1.getting-started/5.theme/3.components.md#L406-L408: replace “the returned classes are the only ones left” with the same qualification.
📍 Affects 2 files
src/runtime/types/tv.ts#L5-L9(this comment)docs/content/docs/1.getting-started/5.theme/3.components.md#L406-L408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/types/tv.ts` around lines 5 - 9, The call-time replacer
documentation must clarify that it replaces resolved classes, while accompanying
plain classes still merge into the result. Update src/runtime/types/tv.ts lines
5-9 and docs/content/docs/1.getting-started/5.theme/3.components.md lines
406-408, replacing the claim that returned classes are the only ones left with
this qualification.
🔗 Linked issue
Resolves #6800
❓ Type of change
📚 Description
A function slot value in
app.config.uiused to replace the slot's fully resolved classes, so it also dropped everything thevariantsandcompoundVariantscontribute. Copying the default theme classes into a replacer produced an unstyled component, which is what #6800 reports.Construction-time replacers are now resolved when the tv config is built: the slot is blanked on the
extendside and the replacer's result takes the place of the slot's own classes, so variants keep merging on top, the same as with a plain string. Thedefaultsargument the replacer receives is now the slot's own theme classes instead of the resolved chain, and(defaults) => defaultsstays a no-op.The
uiandclassprops are unchanged, they run after variant resolution so a function there still replaces the resolved output. Docs now spell out the difference.This also removes the call-time
directivespath entirely, so slots with an app config replacer are memoized again like any other slot.📝 Checklist