fix(alert): propagate className #4535
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 6b78c9d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request addresses a bug in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/hot-owls-sniff.md (1)
1-5: Enhance the changeset descriptionWhile the patch version is correct for this bug fix, consider adding more details about what was fixed and why:
--- "@nextui-org/alert": patch --- -propagate className (#4533) +fix: ensure className prop is correctly propagated to alert base element (#4533) + +Previously, custom classNames weren't being applied to the alert's base element. +This fix ensures that className prop is properly combined with the component's +base classes using clsx.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/hot-owls-sniff.md(1 hunks)packages/components/alert/src/use-alert.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (3)
packages/components/alert/src/use-alert.ts (3)
106-106: LGTM: className prop destructuringThe className prop is correctly destructured from the props object.
127-127: LGTM: Base styles combinationThe baseStyles variable correctly combines the base classNames with the custom className using clsx.
147-148: LGTM: Base props className applicationThe baseStyles are correctly applied using the slots.base utility.
| () => | ||
| alert({hasContent: !isEmpty(description) || !isEmpty(children), ...variantProps, className}), | ||
| [description, objectToDeps(variantProps)], |
There was a problem hiding this comment.
💡 Codebase verification
Dependency array is missing className prop
The review comment is correct. The className prop is used to compute variant-specific styles in the alert function, but it's missing from the useMemo dependency array. This could lead to stale styles when className changes.
🔗 Analysis chain
Verify className dependency in slots memoization
The className is added to the alert variant props, but it's not included in the dependency array of useMemo.
Add className to the dependency array to ensure proper reactivity:
alert({hasContent: !isEmpty(description) || !isEmpty(children), ...variantProps, className}),
- [description, objectToDeps(variantProps)],
+ [description, className, objectToDeps(variantProps)],🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the complete useAlert hook implementation
ast-grep --pattern 'export function useAlert($_) {
$$$
}'
# Also search for any type definitions that might show how className is handled
ast-grep --pattern 'interface AlertProps {
$$$
}'
Length of output: 9194
Closes #4533
📝 Description
Apply className to base (i.e. the same pattern like other components)
⛳️ Current behavior (updates)
classNamedoesn't add to base🚀 New behavior
classNameis added to base💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
@nextui-org/alertpackage