Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Util classname 1 #616

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Util classname 1 #616

merged 4 commits into from
Dec 9, 2024

Conversation

GoldGroove06
Copy link
Contributor

@GoldGroove06 GoldGroove06 commented Dec 9, 2024

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

    • Added a new dependency clsx for improved class name management across various components.
  • Bug Fixes

    • Enhanced class name handling in multiple components, ensuring more flexible and cleaner class name management.
  • Documentation

    • Updated import statements for the clsx utility in relevant components to reflect the new dependency usage.

Copy link
Contributor

coderabbitai bot commented Dec 9, 2024

Warning

Rate limit exceeded

@GoldGroove06 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 11 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between f0d41ef and cb40d86.

📒 Files selected for processing (2)
  • src/components/ui/Accordion/fragments/AccordionRoot.tsx (2 hunks)
  • src/components/ui/Button/Button.tsx (2 hunks)

Walkthrough

The pull request introduces a new dependency, "clsx": "^2.1.1", to the package.json file of the @radui/ui package. This utility is subsequently utilized across multiple components within the project, specifically in files related to the Accordion, AlertDialog, Avatar, Badge, and other UI components. The changes primarily involve replacing existing string interpolation methods for class names with the clsx function, enhancing the management of class names without altering the components' overall functionality or structure.

Changes

File Path Change Summary
package.json Added new dependency: "clsx": "^2.1.1" in dependencies section.
src/components/ui/Accordion/fragments/AccordionContent.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Accordion/fragments/AccordionHeader.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Accordion/fragments/AccordionItem.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Accordion/fragments/AccordionRoot.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Accordion/fragments/AccordionTrigger.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Avatar/Avatar.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/AvatarGroup/AvatarGroup.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Badge/Badge.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Badge/fragments/BadgeRoot.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/BlockQuote/BlockQuote.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Button/Button.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Callout/Callout.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Callout/fragments/CalloutRoot.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Card/Card.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Card/fragments/CardRoot.tsx Updated className handling to use clsx for improved conditional class name management.
src/components/ui/Code/Code.tsx Updated className handling to use clsx for improved conditional class name management.

Possibly related PRs

  • HoverCard Component #583: The changes in the main PR involve adding a new dependency "clsx": "^2.1.1" in package.json, which is relevant to the HoverCard component that utilizes class name management, enhancing its styling and behavior.
  • Toggle Primitive, Toggle, ToggleGroup Refactor #576: The main PR's addition of the "clsx" dependency is related to the refactor of the Toggle and ToggleGroup components, which also focus on improving class name handling and component styling.
  • AvatarGroup Component #503: The introduction of the AvatarGroup component in this PR may utilize the clsx library for managing class names, making it relevant to the changes in the main PR.
  • Refactor badge #454: The refactor of the Badge component may also involve class name management, which connects it to the addition of the clsx dependency in the main PR.
  • AlertDialog Component #448: The AlertDialog component's styling and behavior may benefit from the clsx utility, linking it to the changes in the main PR.

Suggested reviewers

  • kotAPI

🐰 In the meadow, where bunnies play,
A new helper joins us today!
With clsx to tidy our class names right,
Our components shine, oh what a sight!
So hop along, let's celebrate,
For cleaner code, we simply can't wait! 🌼✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (11)
src/components/ui/Code/Code.tsx (1)

3-3: Consider simplifying static className handling

Since you're using a static string 'rui-code-root' without any conditional logic, using clsx here adds unnecessary overhead. You could either:

  1. Keep the direct string assignment if no className customization is needed
  2. Add className prop support for external customization (recommended)
 export type CodeProps = {
     children: React.ReactNode;
+    className?: string;
 }

-const Code = ({ children }: CodeProps) => {
-    return <code className={clsx('rui-code-root')}>
+const Code = ({ children, className }: CodeProps) => {
+    return <code className={clsx('rui-code-root', className)}>

Also applies to: 9-9

src/components/ui/Accordion/fragments/AccordionHeader.tsx (1)

2-2: Consider adding a base className for consistent styling

The component currently only passes through the provided className. Consider adding a base class (e.g., 'rui-accordion-header') to maintain consistent styling across implementations:

-        <div className={clsx(className)}>
+        <div className={clsx('rui-accordion-header', className)}>

Also applies to: 11-11

src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)

11-11: Consider simplifying the clsx usage

Since you're only passing a single template literal, the clsx wrapper might be unnecessary here. However, if you plan to add more conditional classes in the future, the current implementation provides a good foundation.

You could simplify it to:

-                    className={clsx(`${rootClass}-overlay`)} onClick={handleOverlayClick}>
+                    className={`${rootClass}-overlay`} onClick={handleOverlayClick}>

Or if you prefer consistency with other components using clsx:

-                    className={clsx(`${rootClass}-overlay`)} onClick={handleOverlayClick}>
+                    className={clsx(rootClass + '-overlay')} onClick={handleOverlayClick}>
src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)

14-14: Consider simplifying the clsx usage

Similar to AlertDialogOverlay, since you're only passing a single template literal, the clsx wrapper might be unnecessary. However, maintaining consistency across components might be more valuable than the minor optimization.

You could simplify it to:

-                <div className={clsx(`${rootClass}-content`)}>
+                <div className={`${rootClass}-content`}>

Or if you prefer consistency with other components using clsx:

-                <div className={clsx(`${rootClass}-content`)}>
+                <div className={clsx(rootClass + '-content')}>
src/components/ui/BlockQuote/BlockQuote.tsx (1)

Line range hint 11-11: Fix incorrect props type definition.

The props type is incorrectly defined as an array Record<string, any>[] but is used as a single object with the spread operator.

Apply this fix:

-    props: Record<string, any>[]
+    props?: Record<string, any>
src/components/ui/Callout/Callout.tsx (1)

2-2: LGTM with suggestions for improvement.

While the clsx implementation is correct, there are two improvements to consider:

  1. The props type should be a single object, not an array
  2. The color ?? undefined is redundant as undefined is the default for optional props

Apply these improvements:

-    props?: object[]
+    props?: Record<string, any>
-    return (<CalloutRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}>
+    return (<CalloutRoot customRootClass={customRootClass} className={clsx(className)} color={color} {...props}>
         {children}
     </CalloutRoot>);

Also applies to: 18-20

src/components/ui/Callout/fragments/CalloutRoot.tsx (1)

3-3: LGTM! Verify className prop type

The implementation of clsx is correct and maintains existing functionality. However, consider removing the empty string union type from className?: string | '' as clsx already handles empty strings gracefully.

-    className?: string | '' ;
+    className?: string;

Also applies to: 16-16

src/components/ui/Badge/fragments/BadgeRoot.tsx (1)

1-1: Consider standardizing clsx usage patterns

While the implementation is generally good, I noticed some inconsistencies in how clsx is being used across components. Consider:

  1. Standardizing when to use clsx (e.g., only when combining multiple classes)
  2. Adding unit tests specifically for className handling
  3. Creating a shared utility function for common className patterns

Also, please investigate the failing tests mentioned in the PR description, even though they're reportedly unrelated to these changes.

src/components/ui/Avatar/Avatar.tsx (1)

2-2: Simplify unnecessary clsx usage

Using clsx with a single className argument provides no additional benefit and adds unnecessary complexity.

Consider simplifying the code:

-import { clsx } from 'clsx';

// ...

-className={clsx(className)}
+className={className}

Also applies to: 23-23

src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (1)

4-4: Consider simplifying the className handling

The current implementation uses both customClassSwitcher and clsx, but clsx with a single argument doesn't provide any additional benefit. Consider either:

  1. Using only customClassSwitcher if no conditional classes are needed
  2. Combining both custom and conditional classes using clsx in one place
- const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
- <div className={clsx(rootClass)} >
+ <div className={clsx(customClassSwitcher(customRootClass, COMPONENT_NAME))} >

Also applies to: 34-34

package.json (1)

Line range hint 1-1: Overall Implementation Review

The implementation of clsx for className management is clean and consistent across components, with one exception in AccordionRoot.tsx that needs fixing. Please:

  1. Fix the syntax error in AccordionRoot.tsx
  2. Consider adding tests specifically for className combinations to ensure the clsx implementation works as expected

For future maintenance:

  • Consider creating a custom hook or utility function to standardize className composition across components
  • Document the className composition patterns in the component documentation
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 787c234 and f0d41ef.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (21)
  • 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)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/ui/Accordion/fragments/AccordionRoot.tsx

[error] 58-58: expected , but instead found {

Remove {

(parse)

🪛 eslint
src/components/ui/Button/Button.tsx

[error] 24-24: Unexpected template string expression.

(no-template-curly-in-string)

🔇 Additional comments (11)
src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx (2)

2-2: LGTM: Clean import of clsx utility

The import is correctly placed and follows the existing import structure.


14-14: LGTM: Proper implementation of clsx

The clsx implementation correctly handles both the rootClass and optional className prop, maintaining the same functionality while providing better handling of undefined values.

src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)

4-4: LGTM: Clean import of clsx utility

The import is correctly placed and follows the existing import structure.

src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)

3-3: LGTM: Clean import of clsx utility

The import is correctly placed and follows the existing import structure.

src/components/ui/Card/fragments/CardRoot.tsx (1)

3-3: LGTM! Verify className behavior with undefined values.

The implementation correctly uses clsx for className management. The change from template literals to clsx is appropriate and matches the PR objectives.

Let's verify that undefined className values are handled correctly:

Also applies to: 16-16

✅ Verification successful

Let me analyze the className handling implementation and test coverage.


Implementation handles undefined className values correctly

The implementation is safe and properly handles undefined values:

  • Default value className = '' in the props destructuring ensures className is always a string
  • clsx library safely handles undefined/null values by design
  • Test coverage confirms className functionality works as expected
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases involving undefined className values
rg -l "className={undefined}" || rg -l "className: undefined"

Length of output: 177


Script:

#!/bin/bash
# Search for test files related to Card components and check className handling
fd "Card.*test" --exec cat {}

# Search for className usage in Card components
ast-grep --pattern 'className={$_}'

Length of output: 1267


Script:

#!/bin/bash
# Check the actual implementation of className handling in CardRoot
cat src/components/ui/Card/fragments/CardRoot.tsx

Length of output: 684

src/components/ui/BlockQuote/BlockQuote.tsx (1)

3-3: LGTM! className handling is correctly updated.

The implementation correctly uses clsx for className management, consistent with other components.

Also applies to: 17-17

src/components/ui/Badge/fragments/BadgeRoot.tsx (1)

3-3: LGTM! Consistent implementation

The implementation of clsx is correct and consistent with the pattern used in other components. The change maintains existing functionality while improving className handling.

Also applies to: 18-18

src/components/ui/Accordion/fragments/AccordionContent.tsx (1)

4-4: LGTM! Clean implementation of clsx

The changes correctly implement the clsx utility for className management, maintaining the existing functionality while improving class name handling.

Also applies to: 20-20

src/components/ui/AvatarGroup/AvatarGroup.tsx (1)

2-2: LGTM! Clean implementation of clsx

The implementation correctly uses clsx to handle the className prop, allowing for flexible class name combinations while maintaining readability.

Also applies to: 21-21

src/components/ui/Accordion/fragments/AccordionItem.tsx (1)

64-64: LGTM! Clean implementation of clsx

The className implementation correctly combines the base class with the optional className prop using clsx.

package.json (1)

132-132: LGTM! Verify version compatibility

The addition of clsx as a runtime dependency is correct. Version ^2.1.1 is the latest stable version.

Run the following script to verify version compatibility:

✅ Verification successful

LGTM! No compatibility issues found

The verification shows that clsx is being used correctly throughout the UI components for className composition. All usages follow the standard pattern and there are no type errors or compatibility issues detected. The package is properly integrated into the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for version compatibility issues with clsx

# Check if clsx is used in any test files
rg -l "from 'clsx'" -g "*.test.*"

# Check if there are any type errors in files using clsx
rg -l "from 'clsx'" | xargs dirname | sort -u | while read dir; do
  find "$dir" -name "*.d.ts" -o -name "*.ts" -o -name "*.tsx" | while read file; do
    echo "Checking $file for type errors..."
    grep -H "className.*clsx" "$file" || true
  done
done

Length of output: 13064

Comment on lines 4 to 7
export type CardProps = {
customRootClass?: string;
className?: string;
props?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using 'any' type and consider consolidating className props

  1. The props?: any type is not recommended as it bypasses TypeScript's type checking. Consider using a more specific type or removing it if not needed.
  2. Having both className and customRootClass props seems redundant. Consider consolidating them into a single className prop.
 export type CardProps = {
-    customRootClass?: string;
     className?: string;
-    props?: any;
+    props?: React.ComponentProps<'div'>;
 } & React.ComponentProps<'div'>;
📝 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.

Suggested change
export type CardProps = {
customRootClass?: string;
className?: string;
props?: any;
export type CardProps = {
className?: string;
props?: React.ComponentProps<'div'>;
} & React.ComponentProps<'div'>;

Comment on lines 10 to 12
const Card = ({ children, className = '', customRootClass, ...props }: PropsWithChildren<CardProps>) => (
<CardRoot className={className} customRootClass={customRootClass} {...props}>
<CardRoot className={clsx(className)} customRootClass={customRootClass} {...props}>
{children}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify props handling by removing customRootClass

After consolidating the className props, update the component implementation:

-const Card = ({ children, className = '', customRootClass, ...props }: PropsWithChildren<CardProps>) => (
-    <CardRoot className={clsx(className)} customRootClass={customRootClass} {...props}>
+const Card = ({ children, className = '', ...props }: PropsWithChildren<CardProps>) => (
+    <CardRoot className={clsx('rui-card', className)} {...props}>
📝 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.

Suggested change
const Card = ({ children, className = '', customRootClass, ...props }: PropsWithChildren<CardProps>) => (
<CardRoot className={className} customRootClass={customRootClass} {...props}>
<CardRoot className={clsx(className)} customRootClass={customRootClass} {...props}>
{children}
const Card = ({ children, className = '', ...props }: PropsWithChildren<CardProps>) => (
<CardRoot className={clsx('rui-card', className)} {...props}>
{children}

@@ -1,14 +1,14 @@
import React, { PropsWithChildren } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the shell script results, I can now generate the final response:

Inconsistent className handling patterns detected

The codebase shows mixed patterns for handling classNames:

  • Some components use clsx utility: Code.tsx, Badge.tsx, Button.tsx
  • Others use template literals: ToggleGroupRoot.tsx, TextArea.tsx, Strong.tsx
  • Base class naming is also inconsistent:
    • Most components use custom root classes without 'rui-' prefix
    • Only Code.tsx uses the 'rui-' prefix pattern
🔗 Analysis chain

Verify consistent className handling across components

Let's verify that all UI components follow the same pattern for className handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistent className handling patterns across UI components

# Find all UI component files
echo "Checking className handling patterns..."
rg -l "className=" src/components/ui/ | while read -r file; do
    echo "\nFile: $file"
    # Show className related code with context
    rg -A 2 -B 2 "className=" "$file"
done

# Verify base class naming convention
echo "\nChecking base class naming convention..."
rg "className=.*['\"]rui-" src/components/ui/

Length of output: 46635

@@ -1,5 +1,5 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Props typing issues confirmed in multiple components

The following components have incorrect props typing where props is defined as an array Record<string, any>[] instead of a spread parameter type:

  • src/components/ui/Link/Link.tsx
  • src/components/ui/Em/Em.tsx
  • src/components/ui/Kbd/Kbd.tsx
  • src/components/ui/BlockQuote/BlockQuote.tsx

These components use rest parameters (...props) which should be typed as a record object, not an array. The correct type should be Record<string, any> without the array notation.

🔗 Analysis chain

Verify props typing across all components.

Similar props typing issues were found in multiple components. It would be beneficial to verify and fix this pattern across the entire codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potentially incorrect props type definitions
rg "props:.*\[\]" "src/components/"

Length of output: 303


Script:

#!/bin/bash
# Let's check the actual component props definitions in these files
echo "=== Link.tsx ==="
ast-grep --pattern 'interface $_ {
  $$$
}' src/components/ui/Link/Link.tsx

echo -e "\n=== Em.tsx ==="
ast-grep --pattern 'interface $_ {
  $$$
}' src/components/ui/Em/Em.tsx

echo -e "\n=== Kbd.tsx ==="
ast-grep --pattern 'interface $_ {
  $$$
}' src/components/ui/Kbd/Kbd.tsx

echo -e "\n=== BlockQuote.tsx ==="
ast-grep --pattern 'interface $_ {
  $$$
}' src/components/ui/BlockQuote/BlockQuote.tsx

Length of output: 513


Script:

#!/bin/bash
# Let's check the component definitions directly
echo "=== Link.tsx ==="
rg -A 10 "type.*Props.*=|interface.*Props" src/components/ui/Link/Link.tsx

echo -e "\n=== Em.tsx ==="
rg -A 10 "type.*Props.*=|interface.*Props" src/components/ui/Em/Em.tsx

echo -e "\n=== Kbd.tsx ==="
rg -A 10 "type.*Props.*=|interface.*Props" src/components/ui/Kbd/Kbd.tsx

echo -e "\n=== BlockQuote.tsx ==="
rg -A 10 "type.*Props.*=|interface.*Props" src/components/ui/BlockQuote/BlockQuote.tsx

Length of output: 2031

@@ -2,6 +2,7 @@
import React from 'react';
import BadgeRoot from './fragments/BadgeRoot';
import BadgeContent from './fragments/BadgeContent';
import { clsx } from 'clsx';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify unnecessary clsx usage

The current usage of clsx with a single argument is redundant since there's no class name concatenation or conditional logic. You can pass the className directly.

-    return <BadgeRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}>
+    return <BadgeRoot customRootClass={customRootClass} className={className} color={color ?? undefined} {...props}>

Also applies to: 15-15

src/components/ui/Button/Button.tsx Outdated Show resolved Hide resolved
@@ -1,6 +1,7 @@
import React, { useContext } from 'react';
import { AccordionContext } from '../contexts/AccordionContext';
import { AccordionItemContext } from '../contexts/AccordionItemContext';
import { clsx } from 'clsx';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize clsx usage by removing template literal

The current implementation uses a template literal within clsx, but clsx can handle multiple arguments directly for better readability and performance.

- className={clsx(`${rootClass}-trigger`, className)}
+ className={clsx(rootClass + '-trigger', className)}

Or even better, if you want to maintain consistency across the codebase:

- className={clsx(`${rootClass}-trigger`, className)}
+ className={clsx([rootClass + '-trigger', className])}

Also applies to: 35-35


💡 Codebase verification

Template literals in clsx calls need to be replaced with string concatenation

The verification revealed inconsistent className handling patterns across the codebase:

  • Several components are using template literals within clsx calls (e.g., clsx(`${rootClass}-trigger`)), which is unnecessary since clsx already handles string concatenation
  • The pattern appears in multiple files:
    • src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx
    • src/components/ui/Accordion/fragments/AccordionTrigger.tsx
    • src/components/ui/Accordion/fragments/AccordionItem.tsx
    • src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx
    • src/components/ui/Accordion/fragments/AccordionContent.tsx

These should be simplified to clsx(rootClass + '-trigger') or better yet, use string concatenation without template literals: clsx(rootClass + '-trigger', className).

🔗 Analysis chain

Verify consistent className handling patterns

Let's check for consistency in className handling across the codebase.

Also applies to: 35-35

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for different className handling patterns

# Check for template literals within clsx calls
echo "Checking for template literals within clsx..."
rg "clsx\(`\${.*?}`" --type typescript

# Check for direct string concatenation in className
echo "Checking for string concatenation in className..."
rg "className=.*?\+.*?" --type typescript

# Check for potential mixing of different className utilities
echo "Checking for mixed usage of className utilities..."
rg "(clsx|classnames|cn)\(" --type typescript

Length of output: 645


Script:

#!/bin/bash
# Description: Check for different className handling patterns with correct file types

# Check for template literals within clsx calls
echo "Checking for template literals within clsx..."
rg "clsx\(`.*?\$" -t tsx -t jsx -t ts -t js

# Check for direct string concatenation in className
echo "Checking for string concatenation in className..."
rg "className=.*?\+" -t tsx -t jsx -t ts -t js

# Check for potential mixing of different className utilities
echo "Checking for mixed usage of className utilities..."
rg "(clsx|classnames|cn)\(" -t tsx -t jsx -t ts -t js

# Check for files using clsx
echo "Files using clsx..."
fd -e tsx -e jsx -e ts -e js -x rg -l "from ['\"]clsx['\"]"

Length of output: 217


Script:

#!/bin/bash
# Description: Check for different className handling patterns with correct file types

# Check for template literals within clsx calls
echo "Checking for template literals within clsx..."
rg "clsx\(\`" -t tsx -t jsx -t ts -t js

# Check for direct string concatenation in className
echo "Checking for string concatenation in className..."
rg "className=\".*?\+" -t tsx -t jsx -t ts -t js

# Check for potential mixing of different className utilities
echo "Checking for mixed usage of className utilities..."
rg "from ['\"](?:clsx|classnames)['\"]" -t tsx -t jsx -t ts -t js

# List all className assignments
echo "Checking className patterns..."
rg "className=" -t tsx -t jsx -t ts -t js -C 1

Length of output: 713


Script:

#!/bin/bash
# Description: Check for different className handling patterns

# Check for template literals within clsx calls
echo "Checking for template literals within clsx..."
rg "clsx\(\`"

# Check for direct string concatenation in className
echo "Checking for string concatenation in className..."
rg "className=\".*?\+"

# Check for potential mixing of different className utilities
echo "Checking for mixed usage of className utilities..."
rg "from ['\"](?:clsx|classnames)['\"]"

# List files with className assignments
echo "Files with className patterns..."
rg -l "className="

Length of output: 10305

src/components/ui/Accordion/fragments/AccordionRoot.tsx Outdated Show resolved Hide resolved
GoldGroove06 and others added 2 commits December 9, 2024 15:46
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
export type CodeProps= {
children: React.ReactNode;
}

const Code = ({ children }: CodeProps) => {
return <code className='rui-code-root'>
return <code className={clsx('rui-code-root')}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing: customRootClass on Code component

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent use of class prefix - rui is an old format we had used

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore above comments, referenced new issues for them

export type CardProps = {
customRootClass?: string;
className?: string;
props?: any;
} & React.ComponentProps<'div'>;

const Card = ({ children, className = '', customRootClass, ...props }: PropsWithChildren<CardProps>) => (
<CardRoot className={className} customRootClass={customRootClass} {...props}>
<CardRoot className={clsx(className)} customRootClass={customRootClass} {...props}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont need this here

@@ -1,14 +1,14 @@
import React, { PropsWithChildren } from 'react';
import CardRoot from './fragments/CardRoot';

import { clsx } from 'clsx';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed since its direct usage

@@ -15,7 +15,7 @@ export type CalloutProps = {

const COMPONENT_NAME = 'Callout';
const Callout = ({ children, className = '', color, customRootClass, ...props }: CalloutProps) => {
return (<CalloutRoot customRootClass={customRootClass} className={`${className}`} color={color ?? undefined} {...props}>
return (<CalloutRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need here since its direct usage

@@ -11,7 +12,7 @@ export type BadgeProps = {
}

const Badge = ({ children, customRootClass, className, color, ...props }: BadgeProps) => {
return <BadgeRoot customRootClass={customRootClass} className={`${className}`} color={color ?? undefined} {...props}>
return <BadgeRoot customRootClass={customRootClass} className={clsx(className)} color={color ?? undefined} {...props}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need here

@@ -18,7 +18,7 @@ type AvatarGroupProps = {
}

const AvatarGroup = ({ avatars = [], size, customRootClass = '', className, ...props }: AvatarGroupProps) => {
return <AvatarGroupRoot customRootClass={customRootClass} className={className} {...props} >
return <AvatarGroupRoot customRootClass={customRootClass} className={clsx(className)} {...props} >
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need it here

@@ -20,7 +20,7 @@ const Avatar = ({ customRootClass = '', fallback, className, src, alt, ...props
<AvatarPrimitive.Image
src={src}
alt={alt}
className={className}
className={clsx(className)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need

@@ -31,7 +31,7 @@ const AlertDialogRoot = ({ children, customRootClass = '', open, onOpenChange, o
const props = { isOpen, handleOpenChange, floaterContext, rootClass, handleOverlayClick };
return (
<AlertDialogContext.Provider value={props}>
<div className={rootClass} >
<div className={clsx(rootClass)} >
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need

@@ -7,7 +8,7 @@ export type AccordionHeaderProps = {

const AccordionHeader: React.FC<AccordionHeaderProps> = ({ children, className = '' }) => {
return (
<div className={className}>
<div className={clsx(className)}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need

@kotAPI kotAPI merged commit 7dcdcec into rad-ui:main Dec 9, 2024
1 of 3 checks passed
This was referenced Dec 11, 2024
@GoldGroove06 GoldGroove06 deleted the util-classname-1 branch December 12, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants