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.

Sign up for GitHub

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

TextField component #511

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+115 −0
Open

TextField component #511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
021db72
Add TextField component with story for Storybook
kotAPI Nov 1, 2024
ce4e1fe
Refactor TextField component with input fragments
kotAPI Nov 1, 2024
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
28 changes: 28 additions & 0 deletions src/components/ui/TextField/TextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';

import TextFieldRoot from './fragments/TextFieldRoot';
import TextFieldInput from './fragments/TextFieldInput';
import TextFieldSlot from './fragments/TextFieldSlot';
Comment on lines +1 to +5
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 importing types from fragment components.

For better type safety and maintainability, consider importing and re-exporting the types from your fragment components. This would make the API surface more explicit and help with documentation.

 import React from 'react';
 
 import TextFieldRoot from './fragments/TextFieldRoot';
 import TextFieldInput from './fragments/TextFieldInput';
 import TextFieldSlot from './fragments/TextFieldSlot';
+import type { TextFieldRootProps } from './fragments/TextFieldRoot';
+import type { TextFieldInputProps } from './fragments/TextFieldInput';
+import type { TextFieldSlotProps } from './fragments/TextFieldSlot';
+
+export type { TextFieldRootProps, TextFieldInputProps, TextFieldSlotProps };
📝 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
import React from 'react';
import TextFieldRoot from './fragments/TextFieldRoot';
import TextFieldInput from './fragments/TextFieldInput';
import TextFieldSlot from './fragments/TextFieldSlot';
import React from 'react';
import TextFieldRoot from './fragments/TextFieldRoot';
import TextFieldInput from './fragments/TextFieldInput';
import TextFieldSlot from './fragments/TextFieldSlot';
import type { TextFieldRootProps } from './fragments/TextFieldRoot';
import type { TextFieldInputProps } from './fragments/TextFieldInput';
import type { TextFieldSlotProps } from './fragments/TextFieldSlot';
export type { TextFieldRootProps, TextFieldInputProps, TextFieldSlotProps };


type TextFieldProps = React.InputHTMLAttributes<HTMLInputElement> & {
className?: string;
customRootClass?: string;
};

const TextField = ({ className = '', customRootClass = '', value, onChange, ...props }: TextFieldProps) => {
return (
<TextFieldRoot customRootClass={customRootClass} className={className}>
<TextFieldSlot>
slot 1
</TextFieldSlot>
{/* */}
<TextFieldInput type="text" value={value} onChange={onChange} {...props} />
{/* */}
<TextFieldSlot>
slot 2
</TextFieldSlot>
</TextFieldRoot>
);
};
Comment on lines +12 to +26
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

Component implementation needs significant improvements.

Several issues need to be addressed:

  1. Hardcoded slot content ("slot 1", "slot 2")
  2. Missing accessibility attributes
  3. Empty comments in JSX
  4. No error handling

Here's a suggested improvement:

-const TextField = ({ className = '', customRootClass = '', value, onChange, ...props }: TextFieldProps) => {
+const TextField = ({
+    className = '',
+    customRootClass = '',
+    value,
+    onChange,
+    'aria-label': ariaLabel,
+    ...props
+}: TextFieldProps) => {
     return (
-        <TextFieldRoot customRootClass={customRootClass} className={className}>
-            <TextFieldSlot>
-                slot 1
-            </TextFieldSlot>
-            {/*  */}
-            <TextFieldInput type="text" value={value} onChange={onChange} {...props} />
-            {/*  */}
-            <TextFieldSlot>
-                slot 2
-            </TextFieldSlot>
+        <TextFieldRoot customRootClass={customRootClass} className={className} role="group">
+            {props.children || (
+                <>
+                    <TextFieldSlot />
+                    <TextFieldInput
+                        type="text"
+                        value={value}
+                        onChange={onChange}
+                        aria-label={ariaLabel}
+                        {...props}
+                    />
+                    <TextFieldSlot />
+                </>
+            )}
         </TextFieldRoot>
     );
 };
📝 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 TextField = ({ className = '', customRootClass = '', value, onChange, ...props }: TextFieldProps) => {
return (
<TextFieldRoot customRootClass={customRootClass} className={className}>
<TextFieldSlot>
slot 1
</TextFieldSlot>
{/* */}
<TextFieldInput type="text" value={value} onChange={onChange} {...props} />
{/* */}
<TextFieldSlot>
slot 2
</TextFieldSlot>
</TextFieldRoot>
);
};
const TextField = ({
className = '',
customRootClass = '',
value,
onChange,
'aria-label': ariaLabel,
...props
}: TextFieldProps) => {
return (
<TextFieldRoot customRootClass={customRootClass} className={className} role="group">
{props.children || (
<>
<TextFieldSlot />
<TextFieldInput
type="text"
value={value}
onChange={onChange}
aria-label={ariaLabel}
{...props}
/>
<TextFieldSlot />
</>
)}
</TextFieldRoot>
);
};

💡 Codebase verification

Add basic input validation and error handling to TextField component

The TextField component currently lacks input validation and error handling. Based on the codebase analysis:

  • The component extends React.InputHTMLAttributes but doesn't handle common input validation scenarios
  • No error state or error message display mechanism exists
  • No validation patterns or utilities are present in the codebase

Suggested improvements:

  • Add error and errorMessage props to TextFieldProps interface
  • Add visual error state handling in TextFieldRoot and TextFieldInput
  • Implement basic input validation based on HTML5 input attributes (required, pattern, minLength, etc.)
🔗 Analysis chain

Add error handling and validation.

The component should handle common error cases and validate props appropriately.

Let's check if there are any existing error handling patterns in the codebase:

Let me check the error handling pattern found and look for any prop validation patterns in the codebase.

Let me check the specific TextField interface and any existing validation patterns in similar input components.

Let me try one final search to understand the component's interface and validation patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for error handling patterns in other components
rg -g '*.tsx' -g '*.ts' 'try.*catch|throw.*Error|Error\(' src/components/

Length of output: 156

Script:

#!/bin/bash
# Check the specific error handling in ProgressIndicator
rg -A 5 'throw new Error' src/components/ui/Progress/shards/ProgressIndicator.tsx

# Look for prop validation patterns, particularly with PropTypes or TypeScript
ast-grep --pattern 'interface $_Props {
  $$$
}'

# Check for any validation utilities or helper functions
rg -g '*.ts' -g '*.tsx' 'validate|isValid|check' src/

Length of output: 1684

Script:

#!/bin/bash
# Check TextField interface
rg -A 10 'interface TextFieldProps' src/

# Look for validation patterns in input-like components
rg -g '*.tsx' -A 10 'input.*value.*onChange' src/components/

# Check for any existing input validation examples
ast-grep --pattern 'const $_ = ({ value, onChange, $$ }) => {
  $$$
}'

Length of output: 178

Script:

#!/bin/bash
# Search for the TextField type definitions
fd "TextField.tsx" --exec cat {}

# Look for any input validation patterns in the entire codebase
rg -g '*.tsx' -g '*.ts' -B 2 -A 2 'onChange.*=.*\(.*\).*=>' src/

# Check for any prop validation utilities
fd -e ts -e tsx --exec grep -l "validate\|isValid" {} src/utils/

Length of output: 7164


export default TextField;
7 changes: 7 additions & 0 deletions src/components/ui/TextField/fragments/TextFieldInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import React from 'react';

const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
return <input className={`${className}`} {...props} />;
};
Comment on lines +3 to +5
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 integrating with the project's styling system.

The className handling could be improved by integrating with the project's styling system mentioned in the summary (rad-ui-text-field).

-const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
-    return <input className={`${className}`} {...props} />;
+const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
+    return <input className={`rad-ui-text-field__input ${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 TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
return <input className={`${className}`} {...props} />;
};
const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
return <input className={`rad-ui-text-field__input ${className}`} {...props} />;
};

🛠️ Refactor suggestion

Enhance accessibility with ARIA attributes.

Consider adding appropriate ARIA attributes and role to improve accessibility.

 const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
-    return <input className={`${className}`} {...props} />;
+    return (
+        <input
+            className={`${className}`}
+            role="textbox"
+            aria-label={props['aria-label'] || 'text input'}
+            {...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 TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
return <input className={`${className}`} {...props} />;
};
const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
return (
<input
className={`${className}`}
role="textbox"
aria-label={props['aria-label'] || 'text input'}
{...props}
/>
);
};

🛠️ Refactor suggestion

Consider handling controlled component behavior.

Since this is a form input component, it might be helpful to provide some built-in handling for controlled component patterns.

-const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
-    return <input className={`${className}`} {...props} />;
+const TextFieldInput = ({
+    className = '',
+    value,
+    defaultValue,
+    onChange,
+    ...props
+}: TextFieldInputProps) => {
+    const isControlled = value !== undefined;
+    return (
+        <input
+            className={`${className}`}
+            value={isControlled ? value : undefined}
+            defaultValue={!isControlled ? defaultValue : undefined}
+            onChange={(e) => {
+                onChange?.(e);
+            }}
+            {...props}
+        />
+    );

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


export default TextFieldInput;
Comment on lines +1 to +7
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

Export component type for better developer experience.

Consider exporting the component's type definition to improve TypeScript integration.

 import React from 'react';

+export type TextFieldInputProps = React.InputHTMLAttributes<HTMLInputElement>;
+
-const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
+const TextFieldInput = ({ className = '', ...props }: TextFieldInputProps) => {
     return <input className={`${className}`} {...props} />;
 };

 export default TextFieldInput;
📝 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
import React from 'react';
const TextFieldInput = ({ className = '', ...props }: React.InputHTMLAttributes<HTMLInputElement>) => {
return <input className={`${className}`} {...props} />;
};
export default TextFieldInput;
import React from 'react';
export type TextFieldInputProps = React.InputHTMLAttributes<HTMLInputElement>;
const TextFieldInput = ({ className = '', ...props }: TextFieldInputProps) => {
return <input className={`${className}`} {...props} />;
};
export default TextFieldInput;

19 changes: 19 additions & 0 deletions src/components/ui/TextField/fragments/TextFieldRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react';

import { customClassSwitcher } from '~/core';

const COMPONENT_NAME = 'TextField';

type TextFieldRootProps = React.HTMLAttributes<HTMLDivElement> & {
className?: string;
customRootClass?: string;
};

const TextFieldRoot = ({ className = '', customRootClass = '', ...props }: TextFieldRootProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
return <div className={`${rootClass} ${className}`} {...props} />;
};
Comment on lines +12 to +15
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 using a class name utility library.

The current class name concatenation could lead to extra spaces and doesn't handle falsy values. Consider using a utility like clsx or classnames for more robust class name handling.

+import clsx from 'clsx';
+
 const TextFieldRoot = ({ className = '', customRootClass = '', ...props }: TextFieldRootProps) => {
     const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
-    return <div className={`${rootClass} ${className}`} {...props} />;
+    return <div className={clsx(rootClass, className)} {...props} />;
 };

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


TextFieldRoot.displayName = COMPONENT_NAME;

export default TextFieldRoot;
7 changes: 7 additions & 0 deletions src/components/ui/TextField/fragments/TextFieldSlot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import React from 'react';

const TextFieldSlot = ({ children }: React.HTMLAttributes<HTMLDivElement>) => {
return <div className="rad-ui-text-field-slot">{children}</div>;
};
Comment on lines +3 to +5
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

Improve component typing and props handling.

The component could benefit from more precise typing and proper props handling:

  1. The component should explicitly type children and spread other HTML attributes
  2. Consider adding explicit return type annotation

Consider this improved implementation:

-const TextFieldSlot = ({ children }: React.HTMLAttributes<HTMLDivElement>) => {
-    return <div className="rad-ui-text-field-slot">{children}</div>;
+type TextFieldSlotProps = {
+    children: React.ReactNode;
+} & Omit<React.HTMLAttributes<HTMLDivElement>, 'children'>;
+
+const TextFieldSlot: React.FC<TextFieldSlotProps> = ({ children, className, ...props }) => {
+    return (
+        <div 
+            className={['rad-ui-text-field-slot', className].filter(Boolean).join(' ')}
+            {...props}
+        >
+            {children}
+        </div>
+    );
};

This change:

  • Explicitly types the children prop
  • Properly handles className merging
  • Spreads remaining HTML attributes
  • Adds explicit return type annotation
📝 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 TextFieldSlot = ({ children }: React.HTMLAttributes<HTMLDivElement>) => {
return <div className="rad-ui-text-field-slot">{children}</div>;
};
type TextFieldSlotProps = {
children: React.ReactNode;
} & Omit<React.HTMLAttributes<HTMLDivElement>, 'children'>;
const TextFieldSlot: React.FC<TextFieldSlotProps> = ({ children, className, ...props }) => {
return (
<div
className={['rad-ui-text-field-slot', className].filter(Boolean).join(' ')}
{...props}
>
{children}
</div>
);
};


export default TextFieldSlot;
47 changes: 47 additions & 0 deletions src/components/ui/TextField/stories/TextField.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import React from 'react';
import TextField from '../TextField';
import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor';

import Button from '~/components/ui/Button/Button';

export default {
title: 'Components/TextField',
component: TextField,
render: (args) => <Template {...args} />
};

const Template = (args) => {
return <SandboxEditor className="space-y-4 pt-4">
<TextField {...args} />
</SandboxEditor>;
};

// More on writing stories with args: https://storybook.js.org/docs/react/writing-stories/args
export const All = {
args: {
className: ''
}
};
Comment on lines +20 to +24
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

Enhance the basic story to showcase component capabilities.

The current story with an empty className doesn't effectively demonstrate the TextField's features.

Consider expanding the story to show different states and use cases:

 export const All = {
     args: {
-        className: ''
+        placeholder: 'Enter text here',
+        defaultValue: 'Sample text',
+        disabled: false,
+        // Add other relevant props to showcase functionality
     }
 };

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


const WithFormTemplate = (args) => {
const [value, setValue] = React.useState('');
const handleChange = (e) => {
setValue(e.target.value);
};

const handleSubmit = (e) => {
e.preventDefault();
console.log(value);
};
Comment on lines +32 to +35
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 console.log with Storybook action.

Using console.log for form submission isn't ideal in Storybook stories.

+import { action } from '@storybook/addon-actions';
+
 const handleSubmit = (e) => {
     e.preventDefault();
-    console.log(value);
+    action('form-submitted')(value);
 };
📝 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 handleSubmit = (e) => {
e.preventDefault();
console.log(value);
};
import { action } from '@storybook/addon-actions';
const handleSubmit = (e) => {
e.preventDefault();
action('form-submitted')(value);
};


return <SandboxEditor className="space-y-4 pt-4">
<form onSubmit={handleSubmit}>
<TextField {...args} value={value} onChange={handleChange} />
<Button type="submit" onClick={handleSubmit}>Submit</Button>
</form>
</SandboxEditor>;
};

export const WithForm = WithFormTemplate.bind({});
WithForm.args = {
};
6 changes: 6 additions & 0 deletions styles/themes/components/text-field.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.rad-ui-text-field{
display: flex;
border: 1px solid var(--rad-ui-color-gray-1000);
border-radius: 8px;
padding: 8px 12px;
}
1 change: 1 addition & 0 deletions styles/themes/default.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
@import "./components/link.scss";
@import "./components/separator.scss";
@import "./components/tabs.scss";
@import "./components/text-field.scss";
@import "./components/text.scss";
@import "./components/textarea.scss";
@import "./components/progress.scss";
Expand Down
Loading