Update className to use clsx for primitives #630
Conversation
WalkthroughThe pull request introduces a new dependency, Changes
Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
18-18: Consider simplifying the clsx usageWhile the change to use clsx is correct, in this specific case where we're only combining two strings (and one is empty by default), we could simplify this. However, using clsx provides consistency with other components and future-proofs the code for potential additional class names.
Alternative implementation if you want to simplify:
- return <span className={clsx(fallBackRootClass, className)}>{children}</span>; + return <span className={`${fallBackRootClass}${className ? ` ${className}` : ''}`}>{children}</span>;src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx (1)
32-32: Unnecessary use of clsx for single classNameUsing clsx for a single className prop that defaults to empty string adds unnecessary complexity. The clsx utility is most beneficial when combining multiple conditional classes.
Consider simplifying to:
- className={clsx(className)} + className={className}src/core/primitives/Avatar/fragments/AvatarPrimitiveRoot.tsx (1)
2-2: Consider establishing consistent className handling patternsWhile the introduction of clsx is a good improvement, there's an opportunity to establish consistent patterns across components:
- Use direct assignment for single className props
- Use clsx when combining multiple classes
- Consider creating a utility function that wraps clsx for consistent className handling across all components
This would help maintain consistency and make the codebase more maintainable.
Also applies to: 32-32, 42-42
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx(2 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx(2 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveRoot.tsx(2 hunks)
🔇 Additional comments (6)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
2-2: LGTM: Clean import of clsx utility
The import is correctly placed and follows the standard import organization pattern.
src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx (1)
2-2: LGTM: Clean import of clsx utility
The import is correctly placed and follows the standard import organization pattern.
src/core/primitives/Avatar/fragments/AvatarPrimitiveRoot.tsx (2)
2-2: LGTM: Clean import of clsx utility
The import is correctly placed and follows the standard import organization pattern.
42-42: LGTM: Appropriate use of clsx for combining classes
This is a good use case for clsx as it combines the customized root class with the optional className prop. The implementation is clean and handles empty strings appropriately.
Let's verify the customClassSwitcher integration:
✅ Verification successful
Consistent usage of clsx confirmed across Avatar primitive components
The implementation shows a consistent pattern of using clsx for className composition across all Avatar primitive components (Root, Image, and Fallback), following the same clean approach of combining component-specific classes with optional className props.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of customClassSwitcher to ensure consistent pattern
rg "customClassSwitcher" --type ts --type tsx
Length of output: 73
Script:
#!/bin/bash
# Search for other usages of clsx to verify consistent pattern
rg "clsx" -A 2 --type typescript --type-add "typescript:*.{ts,tsx}"
Length of output: 1817
package.json (2)
132-132: LGTM! Verify the clsx version.
The addition of clsx as a runtime dependency is appropriate for className management. Let's verify the version choice.
✅ Verification successful
Version ^2.1.1 is optimal and secure
The specified version ^2.1.1 in package.json is actually using the latest available version (2.1.1) from NPM, and there are no security vulnerabilities reported for the package. The caret (^) notation will allow for compatible minor updates when they become available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest version and security advisories for clsx
# Check NPM for latest versions
echo "Latest version from NPM:"
npm view clsx version
# Check for security advisories
echo -e "\nSecurity advisories from GitHub:"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "clsx") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 507
132-132: Verify the implementation in Avatar components.
Since this PR introduces clsx for className management in Avatar components, let's verify the implementation.
✅ Verification successful
Let me verify the imports with a corrected script to ensure complete verification.
clsx is properly implemented in Avatar components
The verification shows that:
- All Avatar component fragments (Root, Image, Fallback) correctly import and use clsx
- No string concatenations or template literals are used for className
- className implementations properly use clsx for class merging:
- AvatarPrimitiveRoot:
clsx(rootClass, className) - AvatarPrimitiveImage:
clsx(className) - AvatarPrimitiveFallback:
clsx(fallBackRootClass, className)
- AvatarPrimitiveRoot:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of clsx in Avatar components
# Check for string interpolation patterns that should be replaced
echo "Checking for remaining string interpolation in className:"
rg -g "src/core/primitives/Avatar/**/*" "className={\`.*\`}"
# Check for proper clsx usage
echo -e "\nVerifying clsx import and usage:"
ast-grep --pattern 'import { default as clsx } from "clsx"' src/core/primitives/Avatar
# Check className implementation
echo -e "\nChecking className implementation:"
rg -A 3 "className=" src/core/primitives/Avatar
Length of output: 2164
Script:
#!/bin/bash
# Check for clsx imports in Avatar components
echo "Checking clsx imports:"
rg "import.*clsx.*from 'clsx'" src/core/primitives/Avatar
rg 'import.*clsx.*from "clsx"' src/core/primitives/Avatar
# Check for any remaining className concatenations or template literals
echo -e "\nChecking for string concatenations in className:"
rg -g "src/core/primitives/Avatar/**/*" "className=.*\+"
rg -g "src/core/primitives/Avatar/**/*" 'className=.*\${'
# Check all className implementations for completeness
echo -e "\nAll className occurrences with context:"
rg -B 2 -A 2 "className=" src/core/primitives/Avatar
Length of output: 2854
kotAPI
added
the
automerge
2 participants
Purpose
Fixes #613
This PR
Introduces clsx to format the value of the
classNameproperty instead of string interpolation to handle cases such as when the value ofclassNameisundefined.Summary by CodeRabbit
New Features
clsxutility for improved class name handling in various Avatar components.Bug Fixes
AvatarPrimitiveFallback,AvatarPrimitiveImage, andAvatarPrimitiveRootcomponents.Chores
package.jsonto includeclsxas a new dependency.