Util classname 2 #617
Util classname 2 #617
Conversation
|
Warning Rate limit exceeded@GoldGroove06 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces a new dependency, Changes
Possibly related PRs
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (35)
src/components/ui/HoverCard/fragments/HoverCardRoot.tsx (1)
6-6: Optimize the clsx import statementThe current import can be simplified.
-import { clsx } from 'clsx'; +import clsx from 'clsx';src/components/ui/AvatarGroup/AvatarGroup.tsx (1)
Line range hint
11-17: Consider addressing type definition improvements.While not directly related to the className changes, there are two type-related improvements to consider:
- The
sizeprop is defined but not used in the component implementation- Using
Record<string, any>for additional props reduces type safetyConsider these improvements:
type AvatarGroupProps = { avatars: { fallback: string, src: string, alt: string }[]; size: 'sm' | 'md' | 'lg'; - customRootClass?: string; className?: string; - props?: Record<string, any>; + // Use React's HTMLAttributes for additional props + } & React.HTMLAttributes<HTMLDivElement>;Also, either implement the size prop in the component or remove it if it's not needed.
src/components/ui/Collapsible/Collapsible.tsx (4)
17-17: Simplify className implementationUsing
clsxfor a single static class adds unnecessary complexity. Since there's no conditional logic or class merging needed, consider using the className directly:-className={clsx('size-6')} +className="size-6"This change would maintain the same functionality while being more straightforward.
Also applies to: 23-23
Line range hint
41-52: Consider enhancing the component's implementationSeveral improvements could be made to align with best practices:
- Replace inline styles with className-based styling for better maintainability and consistency
- The transition property is incomplete - 'all' without duration or timing function may not work as expected
- Consider implementing RTL support as mentioned in the TODO comments
Here's a suggested approach for the collapsible content:
<div aria-hidden={!open} - style={{ - overflow: 'hidden', - display: 'flex', - flexDirection: 'column', - height: (props.open ?? open) ? 'auto' : '0', - transition: 'all' - }}> + className={clsx( + 'overflow-hidden flex flex-column', + 'transition-all duration-300 ease-in-out', + (props.open ?? open) ? 'h-auto' : 'h-0' + )}>
Line range hint
34-37: Consider improving the header structureThe header section also uses inline styles. Consider converting to className-based styling and improving the semantic structure:
-<span style={{ display: 'flex', alignItems: 'center' }}> +<header className="flex items-center justify-between"> {title && <p>{title}</p>} {trigger || <ButtonPrimitive className="ml-auto" onClick={toggleCollapse}>{open ? <CollapseIcon/> : <ExpandIcon/>}</ButtonPrimitive>} -</span> +</header>This change would:
- Improve semantics with
headertag- Remove inline styles
- Better convey the layout intent
Inline styles should be converted to className-based styling for consistency
The verification reveals inconsistencies in styling approaches across the UI components:
- Multiple components are still using inline styles for layout properties that could be managed through className patterns
- The codebase shows extensive usage of clsx for className management, indicating a clear preference for this approach
Key areas needing attention:
- The
style={{ display: 'flex', alignItems: 'center' }}in Collapsible component should be converted to className- The
style={{ marginInlineStart: 'auto' }}on ButtonPrimitive should use utility classes- Similar inline styles for layout in TreeItem, Dropdown, and other components should be refactored for consistency
🔗 Analysis chain
Line range hint
1-58: Verify consistency of className management across componentsSince this PR is part of a broader effort to improve className management, let's verify the approach is consistent with other components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components using inline styles that could be converted to className rg -t tsx "style={{" src/components/ui/ # Check for other uses of clsx in the codebase to ensure consistent patterns rg -t tsx "className={clsx" src/components/ui/Length of output: 297
Script:
#!/bin/bash # Search for other components using inline styles that could be converted to className rg --type tsx "style=\{\{" src/components/ui/ # Check for other uses of clsx in the codebase to ensure consistent patterns rg --type tsx "className=\{clsx" src/components/ui/Length of output: 157
Script:
#!/bin/bash # Search for other components using inline styles that could be converted to className rg "style=\{\{" --type-add 'tsx:*.tsx' src/components/ui/ # Check for other uses of clsx in the codebase to ensure consistent patterns rg "className=\{clsx" --type-add 'tsx:*.tsx' src/components/ui/Length of output: 7339
src/components/ui/Code/Code.tsx (1)
9-9: Consider simplifying the clsx usageWhile the clsx usage is consistent with other components, using it for a single static className without conditionals or combinations could be simplified. However, keeping it for consistency with the codebase-wide pattern is also valid.
- return <code className={clsx('rui-code-root')}> + return <code className="rui-code-root">src/components/ui/HoverCard/fragments/HoverCardArrow.tsx (1)
9-9: Simplify clsx usage by removing template literalThe template literal inside
clsxis unnecessary sinceclsxalready handles string concatenation efficiently.- return <Floater.Arrow className={clsx(`${rootClass}-arrow`)} {...props} context={floatingContext} ref={arrowRef} />; + return <Floater.Arrow className={clsx(rootClass + '-arrow')} {...props} context={floatingContext} ref={arrowRef} />;src/components/ui/Card/Card.tsx (1)
11-11: Simplify className handlingUsing
clsxwith a single string argument provides no additional benefit over passing the string directly.- <CardRoot className={clsx(className)} customRootClass={customRootClass} {...props}> + <CardRoot className={className} customRootClass={customRootClass} {...props}>src/components/ui/Kbd/Kbd.tsx (1)
Line range hint
8-13: Fix incorrect props type definitionThe
propsproperty inKbdPropsis incorrectly typed as an array of records, which is incompatible with the spread operator usage. It should extend from React's HTMLAttributes.export type KbdProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props: Record<string, any>[]; -} +} & React.HTMLAttributes<HTMLElement>src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx (2)
Line range hint
5-9: Improve type definition for customRootClassThe type
string | ''is unusual and can be simplified to juststringsince an empty string is already included in thestringtype.type AvatarGroupRootProps = { - customRootClass?: string | ''; + customRootClass?: string; children: React.ReactNode; className?: string; }
14-14: Simplify clsx usageThe implementation is correct, but since we're not handling conditional classes, the template literal isn't necessary.
- <div className={clsx(rootClass, className)} {...props}> + <div className={clsx(rootClass, className)} {...props}>src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)
11-11: Simplify clsx usageThe current implementation uses a template literal inside clsx unnecessarily. Since we're concatenating a static suffix, we can simplify this.
- className={clsx(`${rootClass}-overlay`)} onClick={handleOverlayClick}> + className={clsx(rootClass + '-overlay')} onClick={handleOverlayClick}>src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)
14-14: Simplify clsx usageFor consistency with other components, simplify the clsx usage here as well.
- <div className={clsx(`${rootClass}-content`)}> + <div className={clsx(rootClass + '-content')}>src/components/ui/Em/Em.tsx (1)
Line range hint
8-13: Fix Props type definitionThe
propstype is incorrectly defined as an array of Records. For anemelement, it should extend React's HTML attributes.export type EmProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props: Record<string, any>[] -} +} & React.HTMLAttributes<HTMLElement>src/components/ui/Quote/Quote.tsx (1)
Line range hint
8-13: Fix Props type definitionThe
propstype should extend React's quote element attributes for proper typing support.export type QuoteProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props?: Record<string, any>[] -} +} & React.QuoteHTMLAttributes<HTMLQuoteElement>src/components/ui/Card/fragments/CardRoot.tsx (2)
Line range hint
6-10: Improve type safety of propsThe
propstype is too loose withany. Consider extending from appropriate React div element attributes.export type CardRootProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props?: any; -}; +} & React.HTMLAttributes<HTMLDivElement>;
Line range hint
12-12: Remove unnecessary default valueThe
clsxutility handles undefined/empty values gracefully, making the default empty string unnecessary.-const CardRoot = ({ children, customRootClass, className = '', ...props }: CardRootProps) => { +const CardRoot = ({ children, customRootClass, className, ...props }: CardRootProps) => {src/components/ui/BlockQuote/BlockQuote.tsx (1)
Line range hint
9-13: Consider improving type safety of propsThe
propstype usingRecord<string, any>[]is too permissive and could lead to runtime errors. Consider:
- Using a more specific type for props
- Removing the array type as props are typically an object, not an array
export type BlockQuoteProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props: Record<string, any>[] + [key: string]: unknown; // For additional HTML attributes }src/components/ui/Callout/Callout.tsx (2)
18-20: Simplify className handlingThe
clsx(className)call is redundant when passing a single string. Since className already has a default empty string, you can pass it directly.- return (<CalloutRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}> + return (<CalloutRoot customRootClass={customRootClass} className={className} color={color} {...props}> {children} </CalloutRoot>);
Line range hint
8-14: Consider improving type safety for color propThe
colorprop could benefit from a union type of allowed values to prevent invalid colors.export type CalloutProps = { children?: React.ReactNode; className?: string; - color?: string; + color?: 'primary' | 'secondary' | 'warning' | 'error' | 'success'; // Add your supported colors customRootClass?: string; props?: object[] }src/components/ui/Callout/fragments/CalloutRoot.tsx (1)
Line range hint
7-12: Ensure type consistency across componentsThere are some inconsistencies in the type definitions between Callout and CalloutRoot:
classNametype differs:string | ''vs juststringpropstype differs:Record<any, any>[]vsobject[]type CalloutRootProps = { children?: React.ReactNode; - className?: string | '' ; + className?: string; color?: string; customRootClass?: string; - props?: Record<any, any>[] + [key: string]: unknown; // For additional HTML attributes }src/components/ui/Badge/Badge.tsx (1)
15-15: Simplify the color prop handlingThe
?? undefinedcheck is redundant since undefined is the default value when the prop is not provided.- return <BadgeRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}> + return <BadgeRoot customRootClass={customRootClass} className={clsx(className)} color={color} {...props}>src/components/ui/Badge/fragments/BadgeRoot.tsx (1)
18-18: Simplify the color prop handlingThe
?? undefinedcheck is redundant since undefined is the default value when the prop is not provided.- <span className={clsx(rootClass, className)} data-accent-color={color ?? undefined} {...props}> + <span className={clsx(rootClass, className)} data-accent-color={color} {...props}>src/components/ui/Dropdown/Dropdown.tsx (2)
Line range hint
5-8: Address TODO and improve type safetyThe component uses
anytype which reduces type safety. Consider defining proper interfaces for the list items and selected value.export type DropdownProps = { - list: {value: any}[]; - selected: any; + list: Array<{ + value: string | number | React.ReactNode; + id: string | number; + }>; + selected: string | number; }
12-12: Extract hardcoded class names into constantsConsider extracting the hardcoded class names into constants for better maintainability and reusability.
+const DROPDOWN_LIST_CLASSES = 'bg-white px-2 py-2 shadow-lg rounded-md'; +const DROPDOWN_WRAPPER_CLASSES = 'relative'; + const Dropdown = ({ list = [], selected }: DropdownProps) => { const PopElem = () => { - return <ul className={clsx('bg-white px-2 py-2 shadow-lg rounded-md')}> + return <ul className={clsx(DROPDOWN_LIST_CLASSES)}> // ... - return <div className={clsx('relative')}> + return <div className={clsx(DROPDOWN_WRAPPER_CLASSES)}>Also applies to: 18-18
src/components/ui/Link/Link.tsx (2)
18-18: Remove commented out codeSince the alt prop issue has been addressed in the new implementation, this TODO comment and old implementation can be safely removed.
Line range hint
8-14: Remove invalid alt prop from LinkProps typeThe alt prop is noted as invalid for anchor elements, but it's still included in the type definition. Consider removing it from the type definition as well.
export type LinkProps = { children: React.ReactNode; href: string; - alt?: string; customRootClass: string; className: string; props: Record<string, any>[]; }src/components/ui/Accordion/fragments/AccordionContent.tsx (1)
4-4: Simplify clsx usage by removing template literalWhile the current implementation works, we can simplify it by passing the class names directly to clsx without using template literals.
-className={clsx(`${rootClass}-content`, className)} +className={clsx(rootClass + '-content', className)}Or even better, if you prefer more readability:
-className={clsx(`${rootClass}-content`, className)} +className={clsx([rootClass + '-content', className])}Also applies to: 20-20
src/components/ui/Progress/fragments/ProgressIndicator.tsx (1)
4-4: Optimize clsx usageThe current implementation wraps a template literal in clsx, which is unnecessary. You can pass the string directly to clsx.
- className={clsx(`${rootClass}-indicator`)} + className={clsx(rootClass + '-indicator')}Also applies to: 31-31
src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (1)
4-4: Consider simplifying className handlingThe clsx usage here is unnecessary since there's only one class being used. Unless you're planning to add conditional classes in the future, this adds complexity without benefit.
-import { clsx } from 'clsx'; ... - <div className={clsx(rootClass)} > + <div className={rootClass}>Also applies to: 34-34
src/components/ui/Accordion/fragments/AccordionTrigger.tsx (1)
35-35: Optimize clsx usage by removing template literalThe current implementation uses template literal inside clsx which is unnecessary. The clsx utility can handle multiple arguments directly.
- className={clsx(`${rootClass}-trigger`, className)} + className={clsx(rootClass + '-trigger', className)}Or even better, to avoid string concatenation:
- className={clsx(`${rootClass}-trigger`, className)} + className={clsx(rootClass && rootClass + '-trigger', className)}This change makes the code more readable and handles the case where rootClass might be undefined.
src/components/ui/Modal/Modal.tsx (1)
18-19: Simplify className managementThe current usage of
clsxfor static class names doesn't leverage its conditional merging capabilities. Consider using template strings for static classes.-<div className={clsx('modal')}> - <div className={clsx('modal__content')}> +<div className="modal"> + <div className="modal__content">src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
Line range hint
22-22: Remove empty useEffect hookThe useEffect hook is empty and can be removed.
-useEffect(() => {}, []);src/components/ui/Accordion/fragments/AccordionItem.tsx (1)
Line range hint
70-70: Simplify conditional data attributeThe spread operator for conditional data attribute can be simplified using direct attribute assignment.
-{...shouldAddFocusDataAttribute ? { 'data-rad-ui-focus-element': '' } : {}} +data-rad-ui-focus-element={shouldAddFocusDataAttribute ? '' : undefined}
📜 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 (35)
package.json(1 hunks)src/components/ui/Accordion/fragments/AccordionContent.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionHeader.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionItem.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionTrigger.tsx(2 hunks)src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx(2 hunks)src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx(2 hunks)src/components/ui/Avatar/Avatar.tsx(2 hunks)src/components/ui/AvatarGroup/AvatarGroup.tsx(2 hunks)src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx(2 hunks)src/components/ui/Badge/Badge.tsx(2 hunks)src/components/ui/Badge/fragments/BadgeRoot.tsx(2 hunks)src/components/ui/BlockQuote/BlockQuote.tsx(2 hunks)src/components/ui/Button/Button.tsx(2 hunks)src/components/ui/Callout/Callout.tsx(2 hunks)src/components/ui/Callout/fragments/CalloutRoot.tsx(2 hunks)src/components/ui/Card/Card.tsx(1 hunks)src/components/ui/Card/fragments/CardRoot.tsx(2 hunks)src/components/ui/Code/Code.tsx(1 hunks)src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Dropdown/Dropdown.tsx(2 hunks)src/components/ui/Em/Em.tsx(2 hunks)src/components/ui/Heading/Heading.tsx(2 hunks)src/components/ui/HoverCard/fragments/HoverCardArrow.tsx(1 hunks)src/components/ui/HoverCard/fragments/HoverCardContent.tsx(2 hunks)src/components/ui/HoverCard/fragments/HoverCardRoot.tsx(2 hunks)src/components/ui/HoverCard/fragments/HoverCardTrigger.tsx(2 hunks)src/components/ui/Kbd/Kbd.tsx(2 hunks)src/components/ui/Link/Link.tsx(2 hunks)src/components/ui/Modal/Modal.tsx(2 hunks)src/components/ui/Progress/fragments/ProgressIndicator.tsx(2 hunks)src/components/ui/Progress/fragments/ProgressRoot.tsx(2 hunks)src/components/ui/Quote/Quote.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Button/Button.tsx
[error] 24-24: Unexpected template string expression.
(no-template-curly-in-string)
🪛 Biome (1.9.4)
src/components/ui/Accordion/fragments/AccordionRoot.tsx
[error] 58-58: expected , but instead found {
Remove {
(parse)
🔇 Additional comments (32)
src/components/ui/HoverCard/fragments/HoverCardRoot.tsx (2)
Line range hint 1-119: Overall implementation looks solid
The hover card implementation demonstrates good practices:
- Proper state management for controlled/uncontrolled modes
- Well-implemented delay handling for open/close actions
- Clean integration with floating UI
- Good separation of concerns
117-117: 🛠️ Refactor suggestion
Reconsider the clsx usage pattern
The current implementation of clsx(rootClass) with a single argument doesn't leverage clsx's main benefits of handling conditional classes and preventing undefined/null values. Consider either:
- Using rootClass directly if no conditional logic is needed
- Adding conditional classes if needed
Let's check how customClassSwitcher handles undefined/null cases:
If conditional classes are needed, consider this pattern:
-<div className={clsx(rootClass)} {...props}>{children}</div>
+<div
+ className={clsx(
+ rootClass,
+ {
+ 'your-conditional-class': someCondition,
+ // Add more conditional classes as needed
+ }
+ )}
+ {...props}
+>
+ {children}
+</div>✅ Verification successful
Remove unnecessary clsx wrapper
The customClassSwitcher utility already handles undefined/null cases by returning an empty string, making the clsx wrapper redundant when used with a single argument. The utility:
- Returns custom class if provided
- Returns empty string for undefined component name
- Generates prefixed class name for valid component names
Since there's no conditional class logic in the current implementation, you can safely use rootClass directly:
-<div className={clsx(rootClass)} {...props}>{children}</div>
+<div className={rootClass} {...props}>{children}</div>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for customClassSwitcher implementation
ast-grep --pattern 'export const customClassSwitcher = $_' -A 10
Length of output: 1068
src/components/ui/AvatarGroup/AvatarGroup.tsx (1)
2-2: LGTM! Clean import of clsx utility.
The import is correctly placed and uses the recommended named import syntax.
src/components/ui/Button/Button.tsx (1)
4-4: LGTM: Clean clsx import
The import statement is correctly placed and aligns with the PR objective of standardizing className management.
src/components/ui/Collapsible/Collapsible.tsx (1)
3-3: LGTM! Import addition aligns with PR objectives
The addition of the clsx import is consistent with the PR's goal of improving className management.
src/components/ui/Code/Code.tsx (1)
3-3: LGTM: Import statement is correctly placed
The clsx import is properly added at the top of the file with other imports.
src/components/ui/Accordion/fragments/AccordionHeader.tsx (2)
2-2: LGTM: Import statement is correctly placed
The clsx import is properly added at the top of the file.
11-11: LGTM: Proper usage of clsx with optional className
The clsx usage here is appropriate as it handles the optional className prop, ensuring proper class name combination and undefined value handling.
src/components/ui/Progress/fragments/ProgressRoot.tsx (2)
2-2: LGTM: Import statement is correctly placed
The clsx import is properly added at the top of the file.
12-12: Verify customClassSwitcher compatibility with clsx
The integration looks correct, but let's verify that customClassSwitcher always returns a value type that clsx can handle properly.
✅ Verification successful
Let me search for the implementation of customClassSwitcher to verify its return type.
customClassSwitcher is compatible with clsx
The implementation shows that customClassSwitcher always returns a string value (either the custom class, an empty string, or a prefixed component name), which is fully compatible with clsx's type requirements. The function's return type is explicitly typed as :string and all code paths return string values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for customClassSwitcher implementation and usage
ast-grep --pattern 'function customClassSwitcher($_) {
$$$
}'
# Also check other usages for consistency
rg -A 2 'customClassSwitcher\('
Length of output: 9862
Script:
#!/bin/bash
# Search for the implementation file
fd customClassSwitcher.ts
# Once found, let's see its implementation
cat src/core/customClassSwitcher/customClassSwitcher.ts
Length of output: 169
Script:
#!/bin/bash
# Try alternative search patterns to locate the implementation
rg -l 'export.*customClassSwitcher'
# Search for any file containing customClassSwitcher implementation
rg -A 5 'function customClassSwitcher|const customClassSwitcher'
Length of output: 587
Script:
#!/bin/bash
# Let's see the complete implementation of customClassSwitcher
cat src/core/customClassSwitcher/index.ts
# And check how clsx is being used with it in other components
rg -A 2 'clsx\(.*customClassSwitcher'
Length of output: 714
src/components/ui/Card/Card.tsx (1)
11-11: Verify customRootClass handling in CardRoot
Ensure that the customRootClass prop is properly handled in the CardRoot component.
✅ Verification successful
customRootClass is properly handled in CardRoot
The verification shows that the customRootClass prop is correctly implemented in the CardRoot component:
- The prop is properly typed in the CardRootProps interface
- It's correctly passed through the component parameters
- It's properly processed using the
customClassSwitcherutility which:- Returns the custom class if provided
- Falls back to 'rad-ui-card' if not provided
- The resulting class is correctly applied to the root div element using clsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CardRoot implementation to verify customRootClass handling
ast-grep --pattern 'const CardRoot = ({ customRootClass, $$$}) => {
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Let's try different approaches to find CardRoot implementation
# Search for CardRoot styled component declaration
ast-grep --pattern 'const CardRoot = styled.$_'
# Search for CardRoot component declaration with different pattern
ast-grep --pattern 'const CardRoot = $_'
# Search for CardRoot in any form using ripgrep for more context
rg "CardRoot" -A 5
Length of output: 5460
Script:
#!/bin/bash
# Let's verify the customClassSwitcher implementation since it's used for customRootClass handling
rg "customClassSwitcher" -A 3
# Also check for COMPONENT_NAME constant in CardRoot
rg "COMPONENT_NAME.*=.*" src/components/ui/Card/fragments/CardRoot.tsx
Length of output: 22755
src/components/ui/Kbd/Kbd.tsx (1)
16-16: LGTM! Correct usage of clsx
The clsx usage here is appropriate for combining multiple class names.
src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx (1)
2-2: LGTM: Clean import of clsx utility
The import is correctly placed with other dependencies.
src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)
4-4: LGTM: Clean import of clsx utility
The import is correctly placed with other dependencies.
src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)
3-3: LGTM: Clean import of clsx utility
The import is correctly placed with other dependencies.
src/components/ui/Em/Em.tsx (1)
4-4: LGTM! Clean clsx implementation
The migration to clsx is implemented correctly. This change improves className handling by:
- Preventing undefined values
- Eliminating extra spaces
- Providing better type safety
Also applies to: 16-16
src/components/ui/Quote/Quote.tsx (1)
4-4: LGTM! Clean clsx implementation
The migration to clsx is implemented correctly, maintaining consistency with other components.
Also applies to: 16-16
src/components/ui/Card/fragments/CardRoot.tsx (1)
3-3: LGTM! Clean clsx implementation
The migration to clsx is implemented correctly, providing better className handling.
Also applies to: 16-16
src/components/ui/BlockQuote/BlockQuote.tsx (1)
3-3: LGTM: clsx import and usage
The migration to clsx for className management is implemented correctly.
src/components/ui/Callout/fragments/CalloutRoot.tsx (1)
16-18: LGTM: Proper clsx implementation
The migration to clsx for className management is implemented correctly. The data attribute handling for color is also appropriate.
src/components/ui/Badge/Badge.tsx (1)
5-5: LGTM!
The clsx import is correctly added.
src/components/ui/Badge/fragments/BadgeRoot.tsx (1)
3-3: LGTM!
The clsx import is correctly added.
src/components/ui/Dropdown/Dropdown.tsx (1)
3-3: LGTM!
The clsx import is correctly added.
src/components/ui/HoverCard/fragments/HoverCardTrigger.tsx (1)
2-2: LGTM! Clean migration to clsx
The changes correctly implement the clsx utility for className management while maintaining the existing functionality.
Also applies to: 17-17
src/components/ui/Link/Link.tsx (1)
3-3: LGTM! Clean migration to clsx
The changes correctly implement the clsx utility for className management.
Also applies to: 22-22
src/components/ui/Accordion/fragments/AccordionContent.tsx (1)
4-4: Verify consistent clsx implementation across components
Let's verify that all components in the codebase follow the same pattern for className handling.
Also applies to: 20-20
✅ Verification successful
clsx implementation is consistent across UI components
The codebase shows a consistent pattern where:
- All UI components properly import and use
clsxfor className handling - The majority of components use
className={clsx(...)}syntax - Only two minor exceptions found in non-component files (stories and demo) that use string concatenation, which is acceptable for those cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining string interpolation or concatenation of classNames
rg "className=[\`'\"].*\$\{.*\}.*[\`'\"]" --type tsx
rg "className=.*\+" --type tsx
# Search for current clsx usage pattern
rg "className=\{clsx\(" --type tsx
Length of output: 209
Script:
#!/bin/bash
# Search for any remaining string interpolation or concatenation of classNames
rg "className=[\`'\"].*\$\{.*\}.*[\`'\"]"
rg "className=.*\+"
# Search for current clsx usage pattern
rg "className=\{clsx\("
# Also check for imports
rg "import.*clsx"
Length of output: 8329
src/components/ui/Heading/Heading.tsx (1)
3-3: LGTM! Proper implementation of clsx
The changes correctly implement clsx for className management, maintaining consistency across both conditional rendering paths.
Also applies to: 38-38, 40-40
src/components/ui/Progress/fragments/ProgressIndicator.tsx (1)
1-1: Consider standardizing clsx usage patterns
While the migration to clsx is a good improvement, I notice varying implementation patterns across components. Consider establishing consistent patterns for:
- Order of className concatenation
- Handling of conditional classes
- Integration with customClassSwitcher
This will help maintain consistency as more components are migrated.
Let's check the current usage patterns:
src/components/ui/HoverCard/fragments/HoverCardContent.tsx (1)
2-2: LGTM: Import of clsx utility
The import aligns with the PR objective of standardizing className handling.
src/components/ui/Accordion/fragments/AccordionItem.tsx (1)
64-64: LGTM! Good use of clsx for className merging
The clsx usage here correctly merges the base class with optional className prop.
package.json (2)
132-132: LGTM! Good choice of dependency version.
The addition of clsx@^2.1.1 as a direct dependency is appropriate for runtime className management. The caret version range allows for compatible minor updates while maintaining stability.
132-132: Verify security status of clsx dependency.
Let's ensure the chosen version is free from known vulnerabilities.
✅ Verification successful
The clsx dependency version 2.1.1 is secure and up-to-date
The verification shows that:
- 2.1.1 is the latest available version of clsx
- No security vulnerabilities were found in the GitHub Advisory Database for clsx
- npm audit shows no direct security issues with clsx (the reported vulnerabilities are from other dependencies)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and verify latest version of clsx
# Check npm for latest versions and security advisories
npm view clsx versions --json
npm audit info clsx
# Check GitHub Advisory Database
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "clsx") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1841
|
|
||
| const AvatarGroup = ({ avatars = [], size, customRootClass = '', className, ...props }: AvatarGroupProps) => { | ||
| return <AvatarGroupRoot customRootClass={customRootClass} className={className} {...props} > | ||
| return <AvatarGroupRoot customRootClass={customRootClass} className={clsx(className)} {...props} > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve className handling for consistency and clarity.
The current implementation has several areas for improvement:
customRootClassis not being processed by clsx, which could lead to inconsistent handling of class names- Having both
customRootClassandclassNameprops is redundant and could cause confusion - Current clsx usage (
clsx(className)) doesn't provide any additional benefit over using className directly
Consider applying these changes:
- Remove the redundant
customRootClassprop and use onlyclassName - Utilize clsx's ability to handle multiple classes and conditions:
- return <AvatarGroupRoot customRootClass={customRootClass} className={clsx(className)} {...props} >
+ return <AvatarGroupRoot className={clsx(className, customRootClass)} {...props} >Then update the props type:
type AvatarGroupProps = {
avatars: { fallback: string, src: string, alt: string }[];
size: 'sm' | 'md' | 'lg';
- customRootClass?: string;
className?: string;
props?: Record<string, any>;
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -1,5 +1,5 @@ | |||
| import React from 'react'; | |||
|
|
|||
| import { clsx } from 'clsx'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify unnecessary clsx usage and improve props handling
- Using
clsxfor a single className value is unnecessary. You can directly useclassName. - Consider handling
customRootClassconsistently withclsxas well. - The props spreading after className could potentially override it.
import React from 'react';
-import { clsx } from 'clsx';
+import { clsx } from 'clsx';
import AvatarPrimitive from '~/core/primitives/Avatar';
const Avatar = ({ customRootClass = '', fallback, className, src, alt, ...props }: AvatarProps) => {
return (
- <AvatarPrimitive.Root src={src} customRootClass={customRootClass}>
+ <AvatarPrimitive.Root src={src} customRootClass={clsx(customRootClass)}>
<AvatarPrimitive.Image
src={src}
alt={alt}
- className={clsx(className)}
- {...props}
+ {...props}
+ className={className}
/>Also applies to: 23-23
| onPointerEnter={openWithDelay} | ||
| onPointerLeave={closeWithDelay} | ||
| className={`${rootClass}`} {...props} | ||
| className={clsx(rootClass)} {...props} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider props spreading order and className composition
Two suggestions for improvement:
- The props spreading should come before the className to prevent overrides
- The clsx usage could be simplified since there's only one class
- className={clsx(rootClass)} {...props}
+ {...props}
+ className={rootClass}Note: If you plan to add conditional classes in the future, then keeping clsx would make sense. Otherwise, it adds unnecessary complexity for a single class.
📝 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.
| className={clsx(rootClass)} {...props} | |
| {...props} | |
| className={rootClass} |
| <FloatingOverlay className={clsx('overlay')} lockScroll> | ||
| <FloatingFocusManager context={context}> | ||
| <div className='fixed bg-black/80 overflow-auto w-screen h-screen grid place-items-center'> | ||
| <div className='bg-white p-4 inline-block rounded-md shadow-lg'> | ||
| <div className={clsx('fixed bg-black/80 overflow-auto w-screen h-screen grid place-items-center')}> | ||
| <div className={clsx('bg-white p-4 inline-block rounded-md shadow-lg')}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance modal accessibility and className management
The modal overlay and content could benefit from improved accessibility attributes and better className organization.
-<FloatingOverlay className={clsx('overlay')} lockScroll>
+<FloatingOverlay
+ className={clsx('overlay', {
+ 'fixed inset-0': true,
+ 'z-50': true
+ })}
+ lockScroll
+ aria-modal="true"
+ role="dialog"
+>
<FloatingFocusManager context={context}>
- <div className={clsx('fixed bg-black/80 overflow-auto w-screen h-screen grid place-items-center')}>
- <div className={clsx('bg-white p-4 inline-block rounded-md shadow-lg')}>
+ <div className={clsx(
+ 'fixed w-screen h-screen grid place-items-center',
+ 'bg-black/80 overflow-auto'
+ )}>
+ <div className={clsx(
+ 'inline-block',
+ 'bg-white rounded-md shadow-lg',
+ 'p-4'
+ )}>📝 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.
| <FloatingOverlay className={clsx('overlay')} lockScroll> | |
| <FloatingFocusManager context={context}> | |
| <div className='fixed bg-black/80 overflow-auto w-screen h-screen grid place-items-center'> | |
| <div className='bg-white p-4 inline-block rounded-md shadow-lg'> | |
| <div className={clsx('fixed bg-black/80 overflow-auto w-screen h-screen grid place-items-center')}> | |
| <div className={clsx('bg-white p-4 inline-block rounded-md shadow-lg')}> | |
| <FloatingOverlay | |
| className={clsx('overlay', { | |
| 'fixed inset-0': true, | |
| 'z-50': true | |
| })} | |
| lockScroll | |
| aria-modal="true" | |
| role="dialog" | |
| > | |
| <FloatingFocusManager context={context}> | |
| <div className={clsx( | |
| 'fixed w-screen h-screen grid place-items-center', | |
| 'bg-black/80 overflow-auto' | |
| )}> | |
| <div className={clsx( | |
| 'inline-block', | |
| 'bg-white rounded-md shadow-lg', | |
| 'p-4' | |
| )}> |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
This solves the issue #613 by using clsx to handle passing the className. It ensures no blank space or undefined className is passed. The files have been manually checked after the script did the migration. ** The tests would fail because main has some issues **. The tests failing are not related to components changed. Primitives have not been changed
Summary by CodeRabbit
clsxutility for improved class name management across various components.clsxfor class name concatenation, improving readability and maintainability.