Skip to content

Conversation

@hassanzadeh-mj
Copy link

@hassanzadeh-mj hassanzadeh-mj commented Nov 25, 2025

Closes #5908

📝 Description

This PR fixes a bug where defaultVariants.className was being ignored when using the extendVariants helper function. Previously, when extending a component with defaultVariants.className, the className was not being merged correctly with the variant styles and props className.

⛳️ Current behavior (updates)

When using extendVariants with defaultVariants.className, the className was not being applied to the component. The merge logic only considered result (from variant styles) and props.className, completely ignoring defaultVariants.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.className is properly merged with the variant styles and props className. The merge order is:

  1. result - className from variant styles
  2. defaultVariants.className - default className from extendVariants
  3. props.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.className will continue to work exactly as before. Components that were using defaultVariants.className will now work correctly.

📝 Additional Information

Changes Made:

  • Modified getClassNamesWithProps function in extend-variants.js to include defaultVariants?.className in the merge chain
  • Added two comprehensive tests to verify the fix:
    • should respect defaultVariants.className - Verifies default className is applied
    • should merge defaultVariants.className with props.className - Verifies proper merging

Testing:

  • ✅ All existing tests pass (18 tests)
  • ✅ 2 new tests added and passing
  • ✅ Build successful
  • ✅ No linting errors

Related Issue:

This fixes the issue reported in #5908 where defaultVariants.className was being wrongfully overridden.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed className merging for extended components with defaultVariants to properly combine default and prop-based classNames.
  • Tests

    • Added tests verifying defaultVariants.className behavior across different component configurations.

✏️ Tip: You can customize this high-level summary in your review settings.

- 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
@changeset-bot
Copy link

changeset-bot bot commented Nov 25, 2025

⚠️ No Changeset found

Latest commit: 4cb63e0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Nov 25, 2025

@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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

Walkthrough

This pull request fixes a bug in the extendVariants helper where defaultVariants.className was being overridden instead of merged with the generated className and component props. The fix updates the className merging logic for components without slots and adds test coverage for both scenarios.

Changes

Cohort / File(s) Summary
Test additions
packages/core/system-rsc/__tests__/extend-variants.test.tsx
Added two test cases in both no-slots and with-slots describe blocks: "should respect defaultVariants.className" verifies default className is applied, and "should merge defaultVariants.className with props.className" verifies correct merging behavior.
Bug fix
packages/core/system-rsc/src/extend-variants.js
Modified className computation for the no-slots path to merge defaultVariants.className alongside generated result and props.className, fixing the override behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The logic change is straightforward and focused: adding defaultVariants.className to the className merge operation
  • Test cases are well-scoped and verify the expected behavior directly
  • Changes are isolated to a single code path (no-slots scenario)

Suggested reviewers

  • jrgarciadev

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main fix: respecting defaultVariants.className in extendVariants function.
Linked Issues check ✅ Passed The PR successfully addresses the core requirement from issue #5908: merging defaultVariants.className into the final className for components without slots.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the defaultVariants.className bug: modifications to extend-variants.js and corresponding tests added.
Description check ✅ Passed The pull request description thoroughly follows the template with all required sections completed and detailed explanations of the bug, fix, and testing.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@hassanzadeh-mj
Copy link
Author

I would greatly appreciate it if you could kindly review this PR @jrgarciadev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] - getClassNamesWithProps wrongfully overrides given default className

1 participant