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 3 #632

Merged
merged 2 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/ui/Separator/Separator.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import { customClassSwitcher } from '~/core';

import { clsx } from 'clsx';
const COMPONENT_NAME = 'Separator';

export type SeparatorProps = {
Expand All @@ -14,7 +14,7 @@ const Separator = ({ orientation = 'horizontal', className, customRootClass, ...
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
const orientationClass = orientation === 'vertical' ? `${rootClass}-vertical` : `${rootClass}-horizontal`;

return <div className={`${rootClass} ${orientationClass} ${className}`} {...props} ></div>;
return <div className={clsx(rootClass, orientationClass, className)} {...props} ></div>;
};

Separator.displayName = COMPONENT_NAME;
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/Skeleton/Skeleton.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
'use client';
import React from 'react';
import { customClassSwitcher } from '~/core';

import { clsx } from 'clsx';
const COMPONENT_NAME = 'Skeleton';

const Skeleton = ({ loading = true, className = '', customRootClass = '', children, ...props }:any) => {
// If loading is false, return the 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

Replace 'any' type with proper component props interface.

Using 'any' type reduces type safety. Consider defining a proper interface for the component props.

-const Skeleton = ({ loading = true, className = '', customRootClass = '', children, ...props }:any) => {
+interface SkeletonProps extends React.ComponentProps<'span'> {
+  loading?: boolean;
+  className?: string;
+  customRootClass?: string;
+  children?: React.ReactNode;
+}
+
+const Skeleton = ({ loading = true, className = '', customRootClass = '', children, ...props }: SkeletonProps) => {

Committable suggestion skipped: line range outside the PR's diff.

if (!loading) return children;

const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
return <span className={`${rootClass} ${className}`} {...props} >
return <span className={clsx(rootClass, className)} {...props} >
{children}
</span>;
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/Strong/Strong.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';

import { clsx } from 'clsx';
import { customClassSwitcher } from '~/core';
const COMPONENT_NAME = 'Strong';

Expand All @@ -12,7 +12,7 @@ export type StrongProps = {
const Strong = ({ children, className, customRootClass, ...props }: StrongProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
return (
<strong className={`${rootClass} ${className}`} {...props} >{children}</strong>
<strong className={clsx(rootClass, className)} {...props} >{children}</strong>
);
};

Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Switch/Switch.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use client';
import React, { useState } from 'react';
import { customClassSwitcher } from '~/core';
import { clsx } from 'clsx';
const COMPONENT_NAME = 'Switch';

export type SwitchProps = {
Expand All @@ -26,7 +27,7 @@ const Switch = ({ children, customRootClass = '', className = '', color = '', de
};
return (
<>
<input type='checkbox' className={`${rootClass}`} {...props} checked= {isChecked}/>
<input type='checkbox' className={clsx(rootClass)} {...props} checked= {isChecked}/>
<button type="button" onClick={handleChecked} role="switch"></button>
</>
Comment on lines +30 to 32
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

Consider improving accessibility

The current implementation using separate checkbox and button elements might cause accessibility issues. Consider using ARIA attributes and combining the elements.

-            <input type='checkbox' className={clsx(rootClass)} {...props} checked= {isChecked}/>
-            <button type="button" onClick={handleChecked} role="switch"></button>
+            <button
+                type="button"
+                role="switch"
+                aria-checked={isChecked}
+                className={clsx(rootClass, color && `${rootClass}--${color}`)}
+                onClick={handleChecked}
+                {...props}
+            >
+                <span className="sr-only">Toggle switch</span>
+            </button>
📝 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
<input type='checkbox' className={clsx(rootClass)} {...props} checked= {isChecked}/>
<button type="button" onClick={handleChecked} role="switch"></button>
</>
<button
type="button"
role="switch"
aria-checked={isChecked}
className={clsx(rootClass, color && `${rootClass}--${color}`)}
onClick={handleChecked}
{...props}
>
<span className="sr-only">Toggle switch</span>
</button>

);
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Table/fragments/TableBody.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { clsx } from 'clsx';

const TableBody = ({ children, className = '', ...props }:any) => {
return <tbody className={className} {...props} >
return <tbody className={clsx(className)} {...props} >
{children}
</tbody>;
};
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Table/fragments/TableCell.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { clsx } from 'clsx';

const TableCell = ({ children, className = 'cell', ...props }:any) => {
return <td className={className} {...props} >
return <td className={clsx(className)} {...props} >
{children}
</td>;
};
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Table/fragments/TableColumnCellHeader.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { clsx } from 'clsx';

const TableColumnCellHeader = ({ children, className = 'cell-header', ...props }:any) => {
return <th className={className} {...props}>
return <th className={clsx(className)} {...props}>
{children}
</th>;
};
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Table/fragments/TableHead.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { clsx } from 'clsx';

const TableHead = ({ children, className = 'header', ...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

Add proper TypeScript types instead of using any

Using any type defeats TypeScript's type checking benefits. Consider defining proper types:

-const TableHead = ({ children, className = 'header', ...props }:any) => {
+type TableHeadProps = React.ComponentPropsWithoutRef<'thead'> & {
+  children: React.ReactNode;
+  className?: string;
+};
+
+const TableHead = ({ children, className = 'thead-header', ...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.

Suggested change
const TableHead = ({ children, className = 'header', ...props }:any) => {
type TableHeadProps = React.ComponentPropsWithoutRef<'thead'> & {
children: React.ReactNode;
className?: string;
};
const TableHead = ({ children, className = 'thead-header', ...props }: TableHeadProps) => {

return <thead className={className} {...props} >
return <thead className={clsx(className)} {...props} >
{children}
</thead>;
};
Expand Down
6 changes: 3 additions & 3 deletions src/components/ui/Table/fragments/TableRoot.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';

import { clsx } from 'clsx';
import { customClassSwitcher } from '~/core';

const COMPONENT_NAME = 'Table';
Expand All @@ -11,9 +11,9 @@ const TableRoot = ({ children, className = '', customRootClass = '', ...props }:
// so we created a new class for <table> element as a one off case in pattern when it comes to naming classes/conventions
// this is because we cant style the table element directly, so we'll need to wrap it in a div and style it instead

return <div className={`${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={rootClass}>
<table className={clsx(rootClass)}>
{children}
</table>
</div>;
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Table/fragments/TableRow.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { clsx } from 'clsx';

const TableRow = ({ children, className = 'row', ...props }:any) => {
return <tr className={className} {...props} >
return <tr className={clsx(className)} {...props} >
{children}
</tr>;
};
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Tabs/fragments/TabContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React, { useContext } from 'react';
import { customClassSwitcher } from '~/core';
import { TabProps } from '../types';
import TabsRootContext from '../context/TabsRootContext';
import { clsx } from 'clsx';
const COMPONENT_NAME = 'TabContent';

export type TabContentProps ={
Expand All @@ -17,7 +18,7 @@ const TabContent = ({ className, customRootClass }: TabContentProps) => {

const { tabs, activeTab, setActiveTab } = useContext(TabsRootContext);

return <div className={`${rootClass} ${className}`}>
return <div className={clsx(rootClass, className)}>
{tabs.map((tab, index) => {
if (tab.value === activeTab) {
return <div key={index}>{tab.content}</div>;
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/Tabs/fragments/TabList.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';
import React, { useContext } from 'react';

import { clsx } from 'clsx';
import TabTrigger from './TabTrigger';
import { TabProps } from '../types';
import TabsRootContext from '../context/TabsRootContext';
Expand All @@ -17,7 +17,7 @@ export type TabListProps = {

const TabList = ({ className = '', children }: TabListProps) => {
const { rootClass } = useContext(TabsRootContext);
return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={`${rootClass}-list ${className}`}>
return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={clsx(`${rootClass}-list`, className)}>
{children}
</div>;
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/Tabs/fragments/TabRoot.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use client';
import React, { useState, useRef } from 'react';
import { customClassSwitcher } from '~/core';

import { clsx } from 'clsx';
import TabsRootContext from '../context/TabsRootContext';
import { getAllBatchElements, getNextBatchItem, getPrevBatchItem } from '~/core/batches';

Expand Down Expand Up @@ -38,7 +38,7 @@ const TabRoot = ({ children, defaultTab = '', customRootClass, tabs = [], classN
return (
<TabsRootContext.Provider
value={contextValues}>
<div ref={tabRef} className={`${rootClass} ${className}`} data-accent-color={color} {...props} >
<div ref={tabRef} className={clsx(rootClass, className)} data-accent-color={color} {...props} >
{children}
</div>
</TabsRootContext.Provider>
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/Tabs/fragments/TabTrigger.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';
import React, { useContext, useRef } from 'react';

import { clsx } from 'clsx';
import { TabProps } from '../types';

import TabsRootContext from '../context/TabsRootContext';
Expand Down Expand Up @@ -49,7 +49,7 @@ const TabTrigger = ({ tab, className = '', ...props }: TabTriggerProps) => {
return (
<button
ref={ref}
role="tab" className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent}
role="tab" className={clsx(`${rootClass}-trigger`, `${isActive ? 'active' : ''}`, className)} {...props} onKeyDown={handleKeyDownEvent}
onClick={() => handleClick(tab)}
onFocus={() => handleFocus(tab)}
tabIndex={isActive ? 0 : -1}
Expand Down
5 changes: 3 additions & 2 deletions src/components/ui/Text/Text.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
'use client';
import React from 'react';
import { customClassSwitcher } from '~/core';
import { clsx } from 'clsx';

// Can be rendered as p, label, div, span, etc.
// TODO: Add as prop support
// TODO: Add a core reusable function to check and render an as prop

const COMPONENT_NAME = 'Text';

const TAGS = ['div', 'span', 'p', 'label'];
const TAGS = ['div', 'span', 'p', 'label']

export type TextProps = {
children: React.ReactNode;
Expand All @@ -24,7 +25,7 @@ const Text = ({ children, customRootClass = '', className = '', as = 'p', ...pro

return React.createElement(
as,
{ className: `${rootClassName} ${className}`, ...props },
{ className: clsx(rootClassName, className), ...props },
children
);
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/TextArea/TextArea.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';
import React from 'react';

import { clsx } from 'clsx';
import TextAreaRoot from './fragments/TextAreaRoot';
import TextAreaInput from './fragments/TextAreaInput';

Expand All @@ -11,7 +11,7 @@ export type TextAreaProps = {
}

const TextArea = ({ customRootClass = '', className = '', children, ...props }: TextAreaProps) => {
return <TextAreaRoot customRootClass={customRootClass} className={`${className}`}>
return <TextAreaRoot customRootClass={customRootClass} className={clsx(className)}>
<TextAreaInput placeholder="enter text">
{children}
</TextAreaInput>
Comment on lines 15 to 17
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

Make placeholder configurable.

The hardcoded placeholder "enter text" limits the component's reusability. Consider making it a prop.

-        <TextAreaInput placeholder="enter text">
+        <TextAreaInput placeholder={props.placeholder ?? "enter text"}>

And update the TextAreaProps interface:

 export type TextAreaProps = {
     children: React.ReactNode;
     customRootClass?: string;
     className?: string;
+    placeholder?: string;
 }

Committable suggestion skipped: line range outside the PR's diff.

Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/TextArea/fragments/TextAreaRoot.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import { customClassSwitcher } from '~/core';
import { clsx } from 'clsx';

const COMPONENT_NAME = 'TextArea';

Expand All @@ -11,7 +12,7 @@ export type TextAreaProps = {

const TextAreaRoot = ({ children, customRootClass = '', className = '' }:TextAreaProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
return <div className={`${rootClass}${className}`}>
return <div className={clsx(rootClass, className)}>
{children}
</div>;
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/Toggle/Toggle.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useState } from 'react';

import { clsx } from 'clsx';
import { customClassSwitcher } from '~/core';

import TogglePrimitive from '~/core/primitives/Toggle';
Expand Down Expand Up @@ -43,7 +43,7 @@ const Toggle: React.FC<ToggleProps> = ({
return (

<TogglePrimitive
className={`${rootClass}`}
className={clsx(rootClass)}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Include the className prop in clsx call

The className prop is being passed but not utilized in the component's class composition.

Update the className composition:

-className={clsx(rootClass)}
+className={clsx(rootClass, className)}
📝 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
className={clsx(rootClass)}
className={clsx(rootClass, className)}

pressed={isPressed}
onPressedChange={handlePressed}
data-state={isPressed ? 'on' : 'off'}
Expand Down
4 changes: 2 additions & 2 deletions src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useState, useRef } from 'react';

import { clsx } from 'clsx';
import { customClassSwitcher } from '~/core';
import { getAllBatchElements, getNextBatchItem, getPrevBatchItem } from '~/core/batches';

Expand Down Expand Up @@ -38,7 +38,7 @@ const ToggleGroupRoot = ({ type = 'multiple', className = '', loop = true, custo
};

return (
<div className={`${rootClass} ${className}`} role="group" ref={toggleGroupRef}>
<div className={clsx(rootClass, className)} role="group" ref={toggleGroupRef}>
<ToggleContext.Provider
value={sendValues}>
{children}
Expand Down
3 changes: 2 additions & 1 deletion src/components/ui/Tree/fragments/TreeItem.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { useContext, useId, useRef, useState } from 'react';
import ButtonPrimitive from '~/core/primitives/Button';
import { TreeContext } from '../contexts/TreeContext';
import { clsx } from 'clsx';

type TreeItemProps = {
children: React.ReactNode;
Expand Down Expand Up @@ -60,7 +61,7 @@ const TreeItem = ({ children, item, level = 0, className = '', ...props }: TreeI
return <>

<ButtonPrimitive
className={className}
className={clsx(className)}
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

Unnecessary use of clsx

The current implementation clsx(className) doesn't provide any benefits over directly using className since:

  1. No conditional classes are being combined
  2. No template literals are being handled
-className={clsx(className)}
+className={className}

Additionally, consider moving inline styles to className-based styling for better maintainability:

-style={{ display: 'block', alignItems: 'center', gap: '0.5rem' }}
+className={clsx(className, 'tree-item-button')}
-style={{ display: 'flex', alignItems: 'center', gap: '0.5rem' }}
+className="tree-item-content"

This would require adding corresponding CSS classes but would improve maintainability and consistency.

Committable suggestion skipped: line range outside the PR's diff.

ref={buttonRef}
onClick={handleClick}
data-rad-ui-batch-element
Expand Down
Loading