Util function to solve className issue #615
Conversation
WalkthroughThe pull request introduces the 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: 27
🧹 Outside diff range and nitpick comments (42)
src/components/ui/Card/fragments/CardRoot.tsx (1)
Line range hint
1-24: Consider documenting the className pattern.Since this is part of a broader migration to use
clsx, consider adding documentation about this pattern in your component guidelines or README. This will help maintain consistency as new components are added.Example documentation points:
- Why
clsxis preferred over string concatenation- How to handle conditional classes
- Common patterns and edge cases
src/components/ui/Table/fragments/TableRow.tsx (1)
5-7: Add JSDoc documentation for the componentThe component lacks documentation about its purpose and props usage.
+/** + * TableRow component for rendering table rows + * @param {object} props + * @param {React.ReactNode} props.children - The content to be rendered inside the row + * @param {string} [props.className='row'] - Additional CSS classes + */ const TableRow = ({ children, className = 'row', ...props }: TableRowProps) => {src/components/ui/Table/fragments/TableHead.tsx (1)
2-2: Consider creating a shared types file for table componentsTo improve maintainability and ensure consistency across table components, consider:
- Creating a shared types file (
TableTypes.ts)- Defining common interfaces and types
- Implementing a consistent className pattern across all table components
- Adding proper documentation for the entire table component system
This will make the codebase more maintainable and reduce duplication.
src/components/ui/Table/fragments/TableColumnCellHeader.tsx (1)
5-7: Consider extracting the default className to a constants fileThe hardcoded className could be moved to a constants file for better maintainability and reusability.
+// In a constants file +export const TABLE_CLASSES = { + CELL_HEADER: 'cell-header' +} as const; + +// In the component -className = 'cell-header' +className = TABLE_CLASSES.CELL_HEADERsrc/components/ui/Accordion/fragments/AccordionHeader.tsx (3)
Line range hint
4-7: Extend component props with proper HTML attributesThe component should inherit proper HTML attributes for better type safety and functionality.
-export type AccordionHeaderProps = { +export type AccordionHeaderProps = React.HTMLAttributes<HTMLElement> & { children: React.ReactNode; className?: string; }
9-14: Consider using semantic HTML and forwarding remaining propsThe component could benefit from using semantic HTML and properly forwarding HTML attributes.
-const AccordionHeader: React.FC<AccordionHeaderProps> = ({ children, className = '' }) => { +const AccordionHeader: React.FC<AccordionHeaderProps> = ({ + children, + className = 'accordion-header', + ...props +}) => { return ( - <div className={clsx(className)}> + <div className={clsx(className)} {...props}> {children} </div> ); };
AccordionHeader needs ARIA attributes for proper accessibility
The AccordionHeader component should include ARIA attributes to maintain consistency with other accordion components and meet accessibility requirements. Based on the implementation in other components:
- Add
role="heading"and appropriate heading level- Add
id={section-${index}}to match thearia-labelledbyreference in AccordionContent- Consider moving some ARIA attributes from AccordionTrigger if AccordionHeader is meant to be the interactive element
Example locations requiring attention:
src/components/ui/Accordion/fragments/AccordionHeader.tsx: Add ARIA attributes to the root div element- Ensure proper integration with AccordionItem's ID system for ARIA relationships
🔗 Analysis chain
Line range hint
1-17: Verify accessibility requirementsEnsure that the accordion header meets accessibility requirements by using appropriate ARIA attributes.
Let me check the implementation of ARIA attributes in the related accordion components to provide a comprehensive verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other accordion components implement proper ARIA attributes rg -l "aria-" "src/components/ui/Accordion"Length of output: 215
Script:
#!/bin/bash # Check ARIA implementations in accordion components echo "=== AccordionContent.tsx ===" rg "aria-" -A 2 -B 2 "src/components/ui/Accordion/fragments/AccordionContent.tsx" echo -e "\n=== AccordionTrigger.tsx ===" rg "aria-" -A 2 -B 2 "src/components/ui/Accordion/fragments/AccordionTrigger.tsx" echo -e "\n=== AccordionItem.tsx ===" rg "aria-" -A 2 -B 2 "src/components/ui/Accordion/fragments/AccordionItem.tsx"Length of output: 1062
src/components/ui/Progress/fragments/ProgressRoot.tsx (1)
2-2: Consider if clsx is necessary hereThe current implementation wraps a single string value with
clsx, which might be unnecessary sincecustomClassSwitcherlikely returns a properly formatted string already. Theclsxutility is most beneficial when combining multiple classes conditionally.Consider either:
- Using the string directly if no conditional classes are needed:
-return (<div className={clsx(rootClass)}>{children}</div>); +return (<div className={rootClass}>{children}</div>);
- Or leverage clsx's conditional capabilities if needed:
-return (<div className={clsx(rootClass)}>{children}</div>); +return (<div className={clsx(rootClass, { + 'additional-class': someCondition, +})}>{children}</div>);Also applies to: 12-12
src/components/ui/HoverCard/fragments/HoverCardArrow.tsx (1)
2-2: Simplify the className constructionThe current implementation uses a template literal inside
clsx, which can be simplified sinceclsxalready handles string concatenation.-return <Floater.Arrow className={clsx(`${rootClass}-arrow`)} {...props} context={floatingContext} ref={arrowRef} />; +return <Floater.Arrow className={clsx(rootClass + '-arrow')} {...props} context={floatingContext} ref={arrowRef} />;Or even better, use clsx's array syntax for better readability:
-return <Floater.Arrow className={clsx(`${rootClass}-arrow`)} {...props} context={floatingContext} ref={arrowRef} />; +return <Floater.Arrow className={clsx(rootClass, 'arrow')} {...props} context={floatingContext} ref={arrowRef} />;Also applies to: 9-9
src/components/ui/Kbd/Kbd.tsx (1)
Line range hint
8-12: Fix props type definitionThe current props type has an issue:
props: Record<string, any>[]defines props as an array of records- The spread operator usage
{...props}expects a single recordApply this fix:
export type KbdProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props: Record<string, any>[]; + [key: string]: any; }src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx (2)
Line range hint
5-9: Improve type definition for customRootClassThe type definition allows an empty string literal which is redundant since an empty string is already covered by the string type.
type AvatarGroupRootProps = { - customRootClass?: string | ''; + customRootClass?: string; children: React.ReactNode; className?: string; }
14-14: Simplify clsx usageThe template literal within clsx is unnecessary since clsx already handles string concatenation.
- <div className={clsx(rootClass, className)} {...props}> + <div className={clsx(rootClass, className)} {...props}>src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)
11-11: Simplify clsx usage for overlay classNameThe template literal within clsx is unnecessary and can be simplified.
- className={clsx(`${rootClass}-overlay`)} onClick={handleOverlayClick}> + className={clsx(rootClass + '-overlay')} onClick={handleOverlayClick}>src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (2)
14-14: Simplify clsx usage for content classNameThe template literal within clsx is unnecessary and can be simplified.
- <div className={clsx(`${rootClass}-content`)}> + <div className={clsx(rootClass + '-content')}>
Line range hint
1-21: Consider creating a utility function for component class name constructionI notice a pattern across components where class names are constructed by appending suffixes to a root class. Consider creating a utility function to handle this consistently.
Example implementation:
// utils/classNames.ts export const createComponentClassName = (rootClass: string, element: string) => clsx(rootClass + '-' + element); // Usage in components className={createComponentClassName(rootClass, 'content')}This would:
- Standardize class name construction across components
- Make it easier to modify the pattern in the future
- Reduce redundant code
src/components/ui/Em/Em.tsx (1)
Line range hint
8-12: Fix props type definitionThe
propsfield inEmPropsinterface has an incorrect type:
- It's defined as an array
Record<string, any>[]but used as a spread object- It shouldn't be a separate field as props spreading already handles additional properties
Apply this fix:
export type EmProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props: Record<string, any>[] -} +} & React.ComponentProps<'em'>src/components/ui/Quote/Quote.tsx (1)
Line range hint
8-12: Fix props type definitionSimilar to the Em component, the
propsfield inQuotePropsinterface has incorrect typing:
- It's defined as an optional array
props?: Record<string, any>[]but used as a spread object- It shouldn't be a separate field as props spreading already handles additional properties
Apply this fix:
export type QuoteProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props?: Record<string, any>[] -} +} & React.ComponentProps<'q'>src/components/ui/BlockQuote/BlockQuote.tsx (1)
Line range hint
8-13: Fix props type definitionThe
propsfield inBlockQuotePropsis incorrectly typed as an array of records. It should be a single record to match React's prop spreading pattern.export type BlockQuoteProps = { children: React.ReactNode; customRootClass?: string; className?: string; - props: Record<string, any>[] + [key: string]: any; }src/components/ui/Callout/Callout.tsx (1)
Line range hint
8-13: Fix props type definitionSimilar to BlockQuote, the props field should be a record type, not an array.
export type CalloutProps = { children?: React.ReactNode; className?: string; color?: string; customRootClass?: string; - props?: object[] + [key: string]: any; }src/components/ui/Callout/fragments/CalloutRoot.tsx (2)
Line range hint
7-12: Clean up type definitionsThere are several type-related improvements needed:
- The props type should be a record type
- The className type has an unnecessary union with empty string
type CalloutRootProps = { children?: React.ReactNode; - className?: string | ''; + className?: string; color?: string; customRootClass?: string; - props?: Record<any, any>[] + [key: string]: any; }
1-1: Consider creating a shared className utilityGiven the common pattern across components, consider creating a shared utility function to standardize className handling:
// src/utils/className.ts import { clsx, type ClassValue } from 'clsx'; export function cn(...inputs: ClassValue[]) { return clsx(inputs.filter(Boolean)); }This would provide consistent className handling across all components and make it easier to modify the behavior globally if needed.
src/components/ui/Badge/Badge.tsx (1)
15-15: Consider handling undefined className more explicitlyWhile the current implementation works, consider being more explicit about handling undefined className values.
- return <BadgeRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}> + return <BadgeRoot customRootClass={customRootClass} className={clsx(className || '')} color={color ?? undefined} {...props}>src/components/ui/Badge/fragments/BadgeRoot.tsx (1)
1-1: Consider creating a shared className utilityGiven the widespread use of
clsxacross components, consider creating a shared utility function that encapsulates the className handling logic. This could provide:
- Consistent className handling across all components
- Type-safe className composition
- Centralized place for className-related utilities
Example implementation:
// src/utils/className.ts import { clsx, ClassValue } from 'clsx'; export const cn = (...inputs: ClassValue[]) => { return clsx(inputs); };This would allow for consistent usage across components:
import { cn } from '~/utils/className'; // Usage className={cn(rootClass, className)}src/components/ui/Dropdown/Dropdown.tsx (1)
12-12: Simplify clsx usageThe clsx implementation is correct but can be simplified since you're not using conditional classes.
- return <ul className={clsx('bg-white px-2 py-2 shadow-lg rounded-md')}> + return <ul className="bg-white px-2 py-2 shadow-lg rounded-md"> - return <div className={clsx('relative')}> + return <div className="relative">Only use clsx when you need to combine multiple classes conditionally. For static class strings, using the className directly is more straightforward.
Also applies to: 18-18
src/components/ui/Tabs/fragments/TabList.tsx (1)
20-22: Optimize clsx usageThe string interpolation inside clsx is unnecessary as clsx can handle multiple arguments directly.
- return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={clsx(`${rootClass}-list`, className)}> + return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={clsx(rootClass + '-list', className)}>Also, consider providing a more specific aria-label instead of "todo".
src/components/ui/Separator/Separator.tsx (1)
Line range hint
7-11: Improve type safety for propsThe props type uses
anywhich reduces type safety.export type SeparatorProps = { orientation?: 'horizontal' | 'vertical'; className?: string; customRootClass?: string; - props?: any; } & React.ComponentProps<'div'>;The
props?: anyis redundant since you're already extendingReact.ComponentProps<'div'>which provides all valid div props.src/components/ui/Text/Text.tsx (1)
Line range hint
13-19: Consider improving type safety for theasprop.The current implementation uses runtime validation with array includes. This could be improved using TypeScript's literal types for better type safety and developer experience.
Consider updating the types like this:
+type TextTag = 'div' | 'span' | 'p' | 'label'; export type TextProps = { children: React.ReactNode; customRootClass?: string; className?: string; - as?: string; + as?: TextTag; } & React.ComponentProps<'p'>;This would provide better TypeScript support and eliminate the need for runtime validation with
TAGS.includes(as).src/components/ui/Accordion/fragments/AccordionContent.tsx (1)
20-20: Simplify clsx usage patternThe current implementation using template literal inside clsx can be simplified.
-className={clsx(`${rootClass}-content`, className)} +className={clsx(rootClass + '-content', className)}src/components/ui/Table/fragments/TableRoot.tsx (2)
Line range hint
7-7: Replace 'any' type with proper interfaceUsing
anytype reduces type safety. Consider creating a proper interface for the component props.+interface TableRootProps extends React.HTMLAttributes<HTMLDivElement> { + children: React.ReactNode; + className?: string; + customRootClass?: string; +} -const TableRoot = ({ children, className = '', customRootClass = '', ...props }:any) => { +const TableRoot = ({ children, className = '', customRootClass = '', ...props }: TableRootProps) => {
14-18: Simplify clsx usage and address TODO commentThe code can be improved in several ways:
- Simplify the clsx template literal usage
- The TODO comment indicates a need for a wrapper component
-return <div className={clsx(`${rootClass}-wrapper`, className)} {...props} > +return <div className={clsx(rootClass + '-wrapper', className)} {...props} > {/* Todo: need to break this down into its own wrapper component */} - <table className={clsx(rootClass)}> + <table className={rootClass}> {children} </table> </div>;Would you like me to help create a separate TableWrapper component to address the TODO comment?
src/components/ui/Tabs/fragments/TabContent.tsx (1)
21-21: Consider enhancing className handling with conditional classes.While the current implementation works, you could leverage more of clsx's capabilities for conditional classes. For example, you might want to add an 'active' class when content is visible.
- return <div className={clsx(rootClass, className)}> + return <div className={clsx(rootClass, className, { + 'tab-content-active': true, // Add conditional classes as needed + })}>src/components/ui/Heading/Heading.tsx (1)
Line range hint
6-31: Consider simplifying heading level configuration.The
RENDER_AS_ENUMSarray could be simplified to a type-safe object or enum since the label property isn't used.const HEADING_LEVELS = { H1: 'h1', H2: 'h2', H3: 'h3', H4: 'h4', H5: 'h5', H6: 'h6', } as const; type HeadingLevel = typeof HEADING_LEVELS[keyof typeof HEADING_LEVELS];src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (1)
34-34: Consider adding className prop supportWhile the current implementation works, consider adding support for an optional className prop for consistency with other components. This would allow additional classes to be merged with the rootClass.
Here's a suggested implementation:
+type AlertDialogRootProps = { + className?: string; + // ... other props +} -<div className={clsx(rootClass)} > +<div className={clsx(rootClass, className)} >src/components/ui/Toggle/Toggle.tsx (1)
4-4: Consider creating a utility function for className handlingWhile introducing clsx across components is a good step, consider creating a utility function that encapsulates the className merging logic. This would ensure consistent handling across all components and make it easier to modify the behavior in the future.
Example implementation:
// src/utils/className.ts import { clsx, ClassValue } from 'clsx'; export function cn(...inputs: ClassValue[]) { return clsx(...inputs); }This would allow for consistent usage across components:
import { cn } from '~/utils/className'; // In components className={cn(rootClass, className)}Also applies to: 2-2, 4-4
src/components/ui/AvatarGroup/AvatarGroup.tsx (1)
Line range hint
11-17: Improve type definitions for better type safetyThe props interface could be improved to better type the component.
Consider this enhancement:
type AvatarGroupProps = { avatars: { fallback: string, src: string, alt: string }[]; size: 'sm' | 'md' | 'lg'; customRootClass?: string; className?: string; - props?: Record<string, any>; + // Extend from React.HTMLAttributes<HTMLDivElement> for proper HTML props typing + } & Omit<React.HTMLAttributes<HTMLDivElement>, 'className'>;src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
Line range hint
7-7: Add proper TypeScript types instead of using 'any'Using 'any' defeats the purpose of TypeScript's type safety.
Consider this type definition:
type ToggleGroupRootProps = { type?: 'single' | 'multiple'; className?: string; loop?: boolean; customRootClass?: string; componentName?: string; value?: string | string[] | null; children: React.ReactNode; } & Omit<React.HTMLAttributes<HTMLDivElement>, 'className'>;src/components/ui/Accordion/fragments/AccordionTrigger.tsx (1)
Line range hint
38-48: Enhance keyboard interaction handlingThe keyboard interaction handling could be improved to better follow WAI-ARIA practices for accordion components.
Consider adding support for:
- Home key to focus first accordion header
- End key to focus last accordion header
- Space/Enter keys for explicit activation
Example implementation:
const handleKeyDown = (e: React.KeyboardEvent) => { switch (e.key) { case 'ArrowDown': e.preventDefault(); focusNextItem(); break; case 'ArrowUp': e.preventDefault(); focusPrevItem(); break; case 'Home': e.preventDefault(); // Add implementation to focus first item break; case 'End': e.preventDefault(); // Add implementation to focus last item break; } };src/components/ui/Modal/Modal.tsx (1)
18-19: Optimize className management
- Using clsx for static strings without conditionals adds unnecessary complexity.
- Consider extracting Tailwind classes to constants or CSS modules for better maintainability.
- <div className={clsx('modal')}> - <div className={clsx('modal__content')}> + <div className="modal"> + <div className="modal__content"> - <FloatingOverlay className={clsx('overlay')} lockScroll> + <FloatingOverlay className="overlay" lockScroll> - <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="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">Also applies to: 27-30
src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
Line range hint
23-23: Remove empty useEffectThe empty useEffect hook serves no purpose and should be removed.
- useEffect(() => {}, []);src/components/ui/Collapsible/Collapsible.tsx (1)
3-3: Unnecessary use of clsx for static class namesThe current implementation uses
clsxfor single static class names ('size-6'), which doesn't leverage the library's main benefits. Theclsxutility is most valuable when combining multiple classes conditionally.Consider either:
- Keep the static string if no conditional classes are needed:
-className={clsx('size-6')} +className="size-6"
- Or utilize clsx's conditional capabilities if needed:
-className={clsx('size-6')} +className={clsx('size-6', { + 'expanded': open, + 'interactive': !disabled +})}Also applies to: 17-17, 23-23
src/components/ui/Tree/fragments/TreeItem.tsx (1)
1-1: Consider implementing a shared className utilityThe current implementation of
clsxvaries across components. To ensure consistency and maintainability, consider:
- Creating a shared utility function that wraps
clsxwith standardized patterns- Implementing consistent className handling across all components
- Establishing guidelines for when and how to use conditional classes
Example implementation:
// src/utils/className.ts import { clsx, ClassValue } from 'clsx'; export const cn = (...inputs: ClassValue[]) => { return clsx(inputs); };This would provide:
- Consistent className handling across components
- Single point of modification for className logic
- Easy extension for future className utilities
package.json (1)
Line range hint
1-142: Consider updating documentationSince this PR introduces a new utility for className management, consider:
- Adding a note in the README about using clsx for className management
- Updating component documentation to reflect the new className handling approach
📜 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 (56)
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)src/components/ui/RadioGroup/RadioGroup.tsx(2 hunks)src/components/ui/Separator/Separator.tsx(2 hunks)src/components/ui/Skeleton/Skeleton.tsx(1 hunks)src/components/ui/Strong/Strong.tsx(2 hunks)src/components/ui/Switch/Switch.tsx(2 hunks)src/components/ui/Table/fragments/TableBody.tsx(1 hunks)src/components/ui/Table/fragments/TableCell.tsx(1 hunks)src/components/ui/Table/fragments/TableColumnCellHeader.tsx(1 hunks)src/components/ui/Table/fragments/TableHead.tsx(1 hunks)src/components/ui/Table/fragments/TableRoot.tsx(2 hunks)src/components/ui/Table/fragments/TableRow.tsx(1 hunks)src/components/ui/Tabs/fragments/TabContent.tsx(2 hunks)src/components/ui/Tabs/fragments/TabList.tsx(2 hunks)src/components/ui/Tabs/fragments/TabRoot.tsx(2 hunks)src/components/ui/Tabs/fragments/TabTrigger.tsx(2 hunks)src/components/ui/Text/Text.tsx(2 hunks)src/components/ui/TextArea/TextArea.tsx(2 hunks)src/components/ui/TextArea/fragments/TextAreaRoot.tsx(2 hunks)src/components/ui/Toggle/Toggle.tsx(2 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/Tree/fragments/TreeItem.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/components/ui/Table/fragments/TableCell.tsx
- src/components/ui/Tabs/fragments/TabRoot.tsx
- src/components/ui/HoverCard/fragments/HoverCardRoot.tsx
🧰 Additional context used
🪛 eslint
src/components/ui/RadioGroup/RadioGroup.tsx
[error] 5-5: Import in body of module; reorder to top.
(import/first)
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 (29)
src/components/ui/Card/fragments/CardRoot.tsx (2)
3-3: LGTM! Clean import of clsx utility.
The import follows the consistent ordering pattern (React, internal, external).
16-16: Verify className behavior with edge cases.
The change from string concatenation to clsx looks good. However, let's verify the behavior with edge cases to ensure consistent className handling.
✅ Verification successful
Let me gather more information about the className handling patterns, particularly focusing on the clsx usage.
Based on the comprehensive search results, I can now provide the final response:
The className handling with clsx is consistent and safe
The codebase shows a consistent pattern of using clsx for className composition across components, including proper handling of optional className props. The CardRoot component's implementation aligns with the established patterns, where:
- className is properly typed as optional (className?: string)
- clsx is used correctly to merge classNames
- The pattern matches other similar component implementations (e.g., AccordionRoot, TabRoot, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify className handling patterns across components
# and check for potential edge cases in className usage
# Check for any direct string concatenation of classNames that might have been missed
rg -g '*.tsx' -g '*.jsx' '\${.*className.*\}'
# Look for other components that might need similar clsx treatment
ast-grep --pattern 'className={$_}'
# Check for potential edge cases in className prop types
rg -g '*.tsx' -g '*.jsx' 'className[?]?: string'
Length of output: 2638
Script:
#!/bin/bash
# Check for clsx usage patterns
rg -g '*.tsx' -g '*.jsx' 'clsx\(' -A 2
# Check for className concatenation patterns
rg -g '*.tsx' -g '*.jsx' 'className={\`.*\`}' -A 1
# Look for other className utility functions
rg -g '*.tsx' -g '*.jsx' '(classNames|cn)\('
Length of output: 17343
src/components/ui/Table/fragments/TableHead.tsx (1)
2-2: Verify the className patterns across table components
Let's ensure the className patterns are being used consistently across the table components.
Also applies to: 5-5
✅ Verification successful
Let me check how these className values are being used with clsx to ensure consistency.
className patterns are consistent across table components
The verification shows that className patterns are being used consistently across all table components:
- All components use clsx for className composition
- Each component has appropriate default className values
- The implementation follows a uniform pattern where clsx is used to merge the className prop
- TableRoot appropriately handles both wrapper and table classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all table components and their default classNames
echo "Searching for table component default classNames:"
rg -U "className = '[^']*'" src/components/ui/Table/
Length of output: 936
Script:
#!/bin/bash
# Search for clsx usage patterns in table components
echo "Searching for clsx usage in table components:"
rg -U "clsx\(.*\)" src/components/ui/Table/
Length of output: 903
src/components/ui/Card/Card.tsx (1)
3-3: Consider combining className with customRootClass
The current implementation passes className and customRootClass separately. Consider if these should be combined using clsx for more flexible class name management.
Let's check how other components handle these props:
If combining classes is desired, consider:
-<CardRoot className={clsx(className)} customRootClass={customRootClass} {...props}>
+<CardRoot className={clsx(className, customRootClass)} {...props}>Also applies to: 11-11
src/components/ui/TextArea/fragments/TextAreaRoot.tsx (1)
3-3: LGTM! Clean implementation of clsx
The changes correctly implement the clsx utility for className management, maintaining the existing functionality while improving the class name handling.
Also applies to: 15-15
src/components/ui/Kbd/Kbd.tsx (1)
4-4: LGTM! Correct implementation of clsx
The clsx utility is properly implemented for className management.
Also applies to: 16-16
src/components/ui/Skeleton/Skeleton.tsx (1)
4-4: LGTM! Proper implementation of clsx
The clsx utility is correctly implemented for className management.
Also applies to: 12-12
src/components/ui/Em/Em.tsx (1)
4-4: LGTM! Clean implementation of clsx
The change from template literals to clsx is well implemented and aligns with the PR objective of handling undefined values and empty spaces in class names.
Also applies to: 16-16
src/components/ui/Quote/Quote.tsx (1)
4-4: LGTM! Clean implementation of clsx
The change from template literals to clsx is well implemented and aligns with the PR objective.
Also applies to: 16-16
src/components/ui/Strong/Strong.tsx (1)
2-2: LGTM! This is a reference implementation
The Strong component demonstrates the correct implementation pattern:
- Clean clsx usage for className handling
- Proper type safety with
React.ComponentProps<'strong'>
This component can serve as a reference for fixing the type definitions in Em and Quote components.
Also applies to: 15-15
src/components/ui/Badge/Badge.tsx (1)
5-5: LGTM: Clean import addition
The clsx import is correctly added at the top level of the file.
src/components/ui/TextArea/TextArea.tsx (2)
3-3: LGTM: Clean import addition
The clsx import is correctly added at the top level of the file.
14-14: LGTM: Proper className handling
The implementation correctly uses clsx with defaulted className prop, making it safe from undefined values.
src/components/ui/Badge/fragments/BadgeRoot.tsx (2)
3-3: LGTM: Clean import addition
The clsx import is correctly added at the top level of the file.
18-18: Verify consistent className handling across components
The implementation correctly uses clsx to combine rootClass and className. Let's verify this pattern is consistently applied across other components mentioned in the summary.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the className handling pattern across components:
Consistent className handling pattern verified
The implementation using clsx for combining rootClass and className is consistently applied across the component library. Key findings:
- All components use
clsxfor className composition - The pattern
className={clsx(rootClass, className)}is consistently followed - No string concatenation or template literals are used for className handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent className handling pattern across components
# Look for inconsistencies in clsx usage with className and rootClass
# Search for className handling patterns
echo "Checking className handling patterns..."
rg -U "className={(?!clsx).*?}" --type ts --type tsx
# Search for potential string concatenation of classNames that should use clsx
echo "Checking for string concatenation of classNames..."
rg -U "className={\`.*?\`}" --type ts --type tsx
Length of output: 531
Script:
#!/bin/bash
# Search for className patterns in TypeScript/TSX files
echo "Searching for className patterns..."
rg "className=" --type ts --type tsx -A 1
# Search for specific clsx usage
echo "Searching for clsx usage..."
rg "clsx\(" --type ts --type tsx -A 1
# Search for files containing both rootClass and className
echo "Searching for rootClass and className co-occurrence..."
rg "rootClass.*className|className.*rootClass" --type ts --type tsx
Length of output: 500
Script:
#!/bin/bash
# Search for className patterns in all files
echo "Searching for className patterns..."
rg "className=" -A 1
# Search for specific clsx usage
echo "Searching for clsx usage..."
rg "clsx\(" -A 1
# Search for files containing both rootClass and className
echo "Searching for rootClass and className co-occurrence..."
rg "rootClass.*className|className.*rootClass"
Length of output: 65806
src/components/ui/Separator/Separator.tsx (1)
17-17: LGTM! Well-structured clsx implementation
The clsx usage here is clean and properly combines the root class, orientation class, and custom className prop.
src/components/ui/HoverCard/fragments/HoverCardTrigger.tsx (2)
2-2: LGTM! Clean import addition.
The clsx import is properly positioned and follows the import organization pattern.
17-17: LGTM! Proper clsx implementation.
The className handling has been correctly updated to use clsx, maintaining the same parameters while improving the syntax.
src/components/ui/Link/Link.tsx (2)
3-3: LGTM! Clean import addition.
The clsx import is properly positioned and follows the import organization pattern.
22-22: LGTM! Proper clsx implementation.
The className handling has been correctly updated to use clsx, maintaining the same parameters while improving the syntax.
src/components/ui/Text/Text.tsx (2)
4-4: LGTM! Clean import addition.
The clsx import is properly positioned and follows the import organization pattern.
28-28: LGTM! Proper clsx implementation.
The className handling has been correctly updated to use clsx in React.createElement, maintaining the same parameters while improving the syntax.
src/components/ui/Tabs/fragments/TabContent.tsx (1)
6-6: LGTM! Clean import of clsx utility.
The import is correctly placed with other dependencies.
src/components/ui/Switch/Switch.tsx (1)
4-4: LGTM: clsx import added correctly
src/components/ui/Progress/fragments/ProgressIndicator.tsx (1)
31-31: 🛠️ Refactor suggestion
Simplify clsx usage and improve transform calculation
- The template literal inside
clsxis unnecessary - The transform calculation could be more precise with proper bounds checking
- className={clsx(`${rootClass}-indicator`)}
+ className={`${rootClass}-indicator`}Consider improving the transform calculation:
- style={{ transform: `translateX(-${maxValue - value}%)` }}
+ style={{
+ transform: `translateX(-${Math.max(0, Math.min(100, ((maxValue - value) / (maxValue - minValue)) * 100))}%)`
+ }}Likely invalid or redundant comment.
src/components/ui/HoverCard/fragments/HoverCardContent.tsx (1)
2-2: LGTM: clsx import added correctly
src/components/ui/Button/Button.tsx (1)
4-4: LGTM!
The clsx import is correctly added.
src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
2-3: Verify clsx dependency
Let's verify the clsx dependency is properly added to the project.
✅ Verification successful
clsx dependency is properly configured and widely used
The verification shows that:
- clsx is correctly added as a production dependency (version ^2.1.1) in package.json
- The library is consistently imported across 52 UI components, indicating standardized usage throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if clsx is properly added to package.json
cat package.json | jq '.dependencies.clsx, .devDependencies.clsx'
# Find all files using clsx to ensure consistent implementation
rg -l "import.*clsx.*from 'clsx'"
Length of output: 2678
package.json (1)
132-132: Verify clsx version and security status
The addition of clsx looks good. Let's verify the version and security status.
✅ Verification successful
clsx dependency is properly integrated and actively used
The verification shows that:
- The clsx package is properly added to package.json with version ^2.1.1
- No security vulnerabilities were found in GitHub's advisory database
- The library is actively used across 50+ UI components for className management
- The implementation follows a consistent pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify clsx version and check for security advisories
# Check NPM for latest versions
echo "Latest version from NPM:"
npm view clsx version
# Check for security advisories
echo "Security 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: 888
Script:
#!/bin/bash
# Let's try searching for clsx in package-lock.json and yarn.lock to verify the version
echo "Searching in package-lock.json:"
fd "package-lock.json" --exec grep -A 5 '"clsx"' {} \;
echo -e "\nSearching in yarn.lock:"
fd "yarn.lock" --exec grep -A 5 '"clsx@' {} \;
# Let's also check if clsx is actually used in the codebase
echo -e "\nChecking usage of clsx in the codebase:"
rg "from ['|\"]clsx['|\"]" -A 1
Length of output: 9136
| import React from 'react'; | ||
| import { clsx } from 'clsx'; | ||
|
|
||
| const TableRow = ({ children, className = 'row', ...props }:any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript types instead of using 'any'
Using any defeats the purpose of TypeScript. Consider defining proper types for the component props:
-const TableRow = ({ children, className = 'row', ...props }:any) => {
+interface TableRowProps extends React.HTMLAttributes<HTMLTableRowElement> {
+ children: React.ReactNode;
+ className?: string;
+}
+
+const TableRow = ({ children, className = 'row', ...props }: TableRowProps) => {📝 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.
| const TableRow = ({ children, className = 'row', ...props }:any) => { | |
| interface TableRowProps extends React.HTMLAttributes<HTMLTableRowElement> { | |
| children: React.ReactNode; | |
| className?: string; | |
| } | |
| const TableRow = ({ children, className = 'row', ...props }: TableRowProps) => { |
| import React from 'react'; | ||
| import { clsx } from 'clsx'; | ||
|
|
||
| const TableBody = ({ children, className = '', ...props }:any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript types and maintain consistency with TableRow
The component needs proper typing and should follow a consistent pattern with other table components.
-const TableBody = ({ children, className = '', ...props }:any) => {
+interface TableBodyProps extends React.HTMLAttributes<HTMLTableSectionElement> {
+ children: React.ReactNode;
+ className?: string;
+}
+
+const TableBody = ({ children, className = 'body', ...props }: TableBodyProps) => {📝 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.
| const TableBody = ({ children, className = '', ...props }:any) => { | |
| interface TableBodyProps extends React.HTMLAttributes<HTMLTableSectionElement> { | |
| children: React.ReactNode; | |
| className?: string; | |
| } | |
| const TableBody = ({ children, className = 'body', ...props }: TableBodyProps) => { |
| import React from 'react'; | ||
| import { clsx } from 'clsx'; | ||
|
|
||
| const TableHead = ({ children, className = 'header', ...props }:any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript types and ensure consistent className conventions
The component needs proper typing and should follow a consistent pattern with other table components.
-const TableHead = ({ children, className = 'header', ...props }:any) => {
+interface TableHeadProps extends React.HTMLAttributes<HTMLTableSectionElement> {
+ children: React.ReactNode;
+ className?: string;
+}
+
+const TableHead = ({ children, className = 'head', ...props }: TableHeadProps) => {📝 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.
| const TableHead = ({ children, className = 'header', ...props }:any) => { | |
| interface TableHeadProps extends React.HTMLAttributes<HTMLTableSectionElement> { | |
| children: React.ReactNode; | |
| className?: string; | |
| } | |
| const TableHead = ({ children, className = 'head', ...props }: TableHeadProps) => { |
| import React from 'react'; | ||
| import { clsx } from 'clsx'; | ||
|
|
||
| const TableColumnCellHeader = ({ children, className = 'cell-header', ...props }:any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with a proper interface definition
Using any type defeats the purpose of TypeScript's type safety. Consider defining a proper interface for the component props.
-const TableColumnCellHeader = ({ children, className = 'cell-header', ...props }:any) => {
+interface TableColumnCellHeaderProps extends React.ThHTMLAttributes<HTMLTableCellElement> {
+ children: React.ReactNode;
+ className?: string;
+}
+
+const TableColumnCellHeader = ({
+ children,
+ className = 'cell-header',
+ ...props
+}: TableColumnCellHeaderProps) => {📝 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.
| const TableColumnCellHeader = ({ children, className = 'cell-header', ...props }:any) => { | |
| interface TableColumnCellHeaderProps extends React.ThHTMLAttributes<HTMLTableCellElement> { | |
| children: React.ReactNode; | |
| className?: string; | |
| } | |
| const TableColumnCellHeader = ({ | |
| children, | |
| className = 'cell-header', | |
| ...props | |
| }: TableColumnCellHeaderProps) => { |
| return <code className={clsx('rui-code-root')}> | ||
| {children} | ||
| </code>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow className customization while maintaining default styles
The component should merge custom className with default styles.
- return <code className={clsx('rui-code-root')}>
+ return <code className={clsx('rui-code-root', className)} {...props}>
{children}
</code>;Committable suggestion skipped: line range outside the PR's diff.
| <button | ||
| type="button" | ||
| className={`${rootClass}-trigger ${className}`} | ||
| className={clsx(`${rootClass}-trigger`, className)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify clsx usage and enhance accessibility
The current implementation has unnecessary template literals and could benefit from improved accessibility.
Apply these changes:
- className={clsx(`${rootClass}-trigger`, className)}
+ className={clsx(
+ `${rootClass}-trigger`,
+ className
+ )}Also, consider adding these accessibility enhancements:
<button
type="button"
className={clsx(`${rootClass}-trigger`, className)}
+ role="button"
+ aria-pressed={activeItem === itemValue}
onBlur={handleBlurEvent}Committable suggestion skipped: line range outside the PR's diff.
| 'use client'; | ||
| import React, { useContext, useRef } from 'react'; | ||
|
|
||
| import { clsx } from 'clsx'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve clsx usage for better className management
The current implementation doesn't fully utilize clsx's capabilities. Instead of string interpolation, use clsx's object syntax for conditional classes.
- className={clsx(`${rootClass}-trigger`, `${isActive ? 'active' : ''}`, className)}
+ className={clsx(
+ `${rootClass}-trigger`,
+ {
+ 'active': isActive
+ },
+ className
+ )} {...props}Also applies to: 52-52
|
|
||
| }}> | ||
| <div className={`${rootClass}-root`} ref={accordionRef} > | ||
| <div className={clsx(${rootClass}-root)} ref={accordionRef} > |
There was a problem hiding this comment.
Fix syntax error in className
There's a syntax error in the template literal usage within clsx.
- <div className={clsx(${rootClass}-root)} ref={accordionRef} >
+ <div className={clsx(`${rootClass}-root`)} ref={accordionRef} >📝 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.
| <div className={clsx(${rootClass}-root)} ref={accordionRef} > | |
| <div className={clsx(`${rootClass}-root`)} ref={accordionRef} > |
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: expected , but instead found {
Remove {
(parse)
| @@ -1,5 +1,5 @@ | |||
| import React, { useState, useContext, useId, useEffect, useRef } from 'react'; | |||
|
|
|||
| import { clsx } from 'clsx'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve clsx implementation to avoid template literals
The current implementation uses template literals inside clsx, which is unnecessary and doesn't leverage the library's string concatenation capabilities.
Replace the template literal with direct string arguments:
-className={clsx(`${rootClass}-item`, className)}
+className={clsx(rootClass && `${rootClass}-item`, className)}This change:
- Makes the code more readable
- Properly handles cases where rootClass might be undefined
- Better utilizes clsx's built-in string concatenation
Also applies to: 64-64
| import React, { useContext, useId, useRef, useState } from 'react'; | ||
| import ButtonPrimitive from '~/core/primitives/Button'; | ||
| import { TreeContext } from '../contexts/TreeContext'; | ||
| import { clsx } from 'clsx'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance className implementation with conditional states
The current implementation doesn't fully utilize clsx's capabilities for managing conditional classes based on component states.
Consider combining the className with state-based classes:
-className={clsx(className)}
+className={clsx(
+ className,
+ {
+ 'is-toggled': isToggled,
+ 'is-focused': isFocused,
+ [`level-${level}`]: true
+ }
+)}This change:
- Makes the component's visual states more maintainable
- Provides better CSS targeting capabilities
- Properly utilizes clsx's conditional class merging
Also applies to: 64-64
|
This wont be merged. Need to make batches of 10 -15 components |
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
New Features
clsxutility for improved conditional class name management across multiple components.Bug Fixes
Documentation
clsxin various components for clarity on class name management improvements.