-
-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(system-rsc): respect defaultVariants.className in extendVariants #5940
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: canary
Are you sure you want to change the base?
fix(system-rsc): respect defaultVariants.className in extendVariants #5940
Conversation
- Fix getClassNamesWithProps to merge defaultVariants.className with result and props.className - Add tests to verify defaultVariants.className is respected and merged correctly - Fixes heroui-inc#5908
|
|
@hassanzadeh-mj is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request fixes a bug in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
I would greatly appreciate it if you could kindly review this PR @jrgarciadev |
getClassNamesWithPropswrongfully overrides given defaultclassName#5908Closes #5908
📝 Description
This PR fixes a bug where
defaultVariants.classNamewas being ignored when using theextendVariantshelper function. Previously, when extending a component withdefaultVariants.className, the className was not being merged correctly with the variant styles and props className.⛳️ Current behavior (updates)
When using
extendVariantswithdefaultVariants.className, the className was not being applied to the component. The merge logic only consideredresult(from variant styles) andprops.className, completely ignoringdefaultVariants.className.Example:
const ActionButton = extendVariants(Button, {
defaultVariants: {
className: 'w-full text-medium rounded-small',
color: 'primary',
variant: 'solid',
},
});
// The className 'w-full text-medium rounded-small' was not applied## 🚀 New behavior
Now
defaultVariants.classNameis properly merged with the variant styles and props className. The merge order is:result- className from variant stylesdefaultVariants.className- default className from extendVariantsprops.className- className from component props (can override)Example:
const ActionButton = extendVariants(Button, {
defaultVariants: {
className: 'w-full text-medium rounded-small',
color: 'primary',
variant: 'solid',
},
});
// Now the className 'w-full text-medium rounded-small' is properly applied
// And can be merged with additional className from props
// Results in: 'w-full text-medium rounded-small px-4 py-2'## 💣 Is this a breaking change (Yes/No):
No - This is a bug fix that restores the expected behavior. Components that were not using
defaultVariants.classNamewill continue to work exactly as before. Components that were usingdefaultVariants.classNamewill now work correctly.📝 Additional Information
Changes Made:
getClassNamesWithPropsfunction inextend-variants.jsto includedefaultVariants?.classNamein the merge chainshould respect defaultVariants.className- Verifies default className is appliedshould merge defaultVariants.className with props.className- Verifies proper mergingTesting:
Related Issue:
This fixes the issue reported in #5908 where
defaultVariants.classNamewas being wrongfully overridden.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.