feat(Theme): add variants prop for component theming#6031
feat(Theme): add variants prop for component theming#6031Bobakanoosh wants to merge 1 commit intonuxt:v4from
Conversation
|
@benjamincanac Here is a POC of the Seeking guidance on the following, the types are a bit tricky:
|
📝 WalkthroughWalkthroughThis change introduces a new variant theming system for components by adding a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/runtime/composables/useComponentVariant.ts`:
- Around line 32-39: The computed in useComponentVariant currently merges theme
overrides with props.variants which doesn't exist, so component props (e.g.,
color, variant) are ignored and not tracked; update the merge to use the actual
props object (e.g., defu(props, themeOverrides)) so explicit props take
precedence and Vue reactivity tracks prop changes inside the computed returned
by useComponentVariant; optionally, to reduce over-tracking, pick only the
variant-related keys from props before calling defu.
🧹 Nitpick comments (1)
src/runtime/components/Theme.vue (1)
20-26: Consider whether nested<UTheme>should merge contexts.Currently, each
<UTheme>fully replaces both the UI and variant contexts for its descendants. If a user nests<UTheme :variants="{ button: { color: 'red' } }">inside<UTheme :variants="{ button: { variant: 'soft' } }">, the inner theme will lose the outer'svariant: 'soft'.This may be intentional for a POC, but worth considering whether
defu-merging with the parent context (similar to howuseComponentVariantmerges props with theme overrides) would provide a more intuitive cascading behavior.
| export function useComponentVariant<C extends keyof UIConfig>(name: C, props: ComponentVariantProps<C>): ComputedRef<ThemeVariantOverrides<ComponentVariants<C>>> { | ||
| const { variant } = injectVariantContext({ variant: computed(() => ({})) }) | ||
|
|
||
| return computed(() => { | ||
| const themeOverrides = (get(variant.value, name as string) || {}) | ||
|
|
||
| return defu(props.variants ?? {}, themeOverrides) | ||
| }) |
There was a problem hiding this comment.
Bug: props.variants is always undefined — explicit component props are ignored.
ComponentVariantProps<C> maps variant keys (color, variant, block, square, …) as top-level properties. There is no .variants key on this type or on the actual ButtonProps object passed by the caller. As a result, props.variants ?? {} always evaluates to {}, and defu({}, themeOverrides) returns only the theme overrides — making it impossible for explicit component props (e.g., <UButton color="red">) to override theme-provided values.
Additionally, since no variant-related prop is actually read inside this computed, Vue won't track changes to props like color or variant, so the returned value won't reactively update when those props change.
Proposed fix — merge `props` directly with theme overrides
export function useComponentVariant<C extends keyof UIConfig>(name: C, props: ComponentVariantProps<C>): ComputedRef<ThemeVariantOverrides<ComponentVariants<C>>> {
const { variant } = injectVariantContext({ variant: computed(() => ({})) })
return computed(() => {
const themeOverrides = (get(variant.value, name as string) || {})
- return defu(props.variants ?? {}, themeOverrides)
+ return defu(props, themeOverrides)
})
}With defu(props, themeOverrides):
- Explicit prop values (e.g.,
color="red") win over theme overrides. undefinedprops (not passed by parent) fall through to theme overrides, thanks todefuskippingundefined.- Vue's reactivity tracking works because
defuenumerates the reactivepropsproxy.
If you want to limit over-tracking (any prop change triggers recompute), consider picking only variant-relevant keys before merging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function useComponentVariant<C extends keyof UIConfig>(name: C, props: ComponentVariantProps<C>): ComputedRef<ThemeVariantOverrides<ComponentVariants<C>>> { | |
| const { variant } = injectVariantContext({ variant: computed(() => ({})) }) | |
| return computed(() => { | |
| const themeOverrides = (get(variant.value, name as string) || {}) | |
| return defu(props.variants ?? {}, themeOverrides) | |
| }) | |
| export function useComponentVariant<C extends keyof UIConfig>(name: C, props: ComponentVariantProps<C>): ComputedRef<ThemeVariantOverrides<ComponentVariants<C>>> { | |
| const { variant } = injectVariantContext({ variant: computed(() => ({})) }) | |
| return computed(() => { | |
| const themeOverrides = (get(variant.value, name as string) || {}) | |
| return defu(props, themeOverrides) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@src/runtime/composables/useComponentVariant.ts` around lines 32 - 39, The
computed in useComponentVariant currently merges theme overrides with
props.variants which doesn't exist, so component props (e.g., color, variant)
are ignored and not tracked; update the merge to use the actual props object
(e.g., defu(props, themeOverrides)) so explicit props take precedence and Vue
reactivity tracks prop changes inside the computed returned by
useComponentVariant; optionally, to reduce over-tracking, pick only the
variant-related keys from props before calling defu.
❓ Type of change
📚 Description
Follow-up to #4387, adding a
variantsprop to UTheme as discussed.📝 Checklist