Enhance TextField component with ref support and update stories#96
Conversation
- Refactored TextField to use React.forwardRef for better ref handling. - Added a new example in the storybook to demonstrate the ref functionality with a focus button. - Updated TextInput to also support refs, ensuring consistent behavior across components.
|
|
WalkthroughThe changes refactor several text field components to support React ref forwarding by explicitly accepting and passing refs to underlying input elements. A new example demonstrates using a ref to programmatically focus an input. Component signatures and prop types are updated to enable this functionality, along with adding display names for better identification. Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant TextField
participant TextInput
participant InputElement
ParentComponent->>TextField: Pass ref via prop
TextField->>TextInput: Forward ref
TextInput->>InputElement: Attach ref to <input>
ParentComponent->>InputElement: Call focus() via ref
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/src/remix-hook-form/text-field.stories.tsx (1)
64-68: Consider adding refExample to form schema.The ref functionality demonstration is well implemented with proper optional chaining. However, the
refExamplefield is not included in the form schema, so it won't be validated or submitted with the form.If this is intentional for demo purposes, consider adding a comment to clarify. Otherwise, add it to the schema:
const formSchema = z.object({ username: z.string().min(3, 'Username must be at least 3 characters'), price: z.string().min(1, 'Price is required'), email: z.string().email('Invalid email address'), measurement: z.string().min(1, 'Measurement is required'), + refExample: z.string().optional(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/docs/src/remix-hook-form/text-field.stories.tsx(3 hunks)packages/components/src/remix-hook-form/text-field.tsx(2 hunks)packages/components/src/ui/text-field.tsx(2 hunks)packages/components/src/ui/text-input.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jaruesink
PR: lambda-curry/forms#14
File: packages/components/src/ui/textarea.tsx:17-24
Timestamp: 2024-11-26T05:28:58.779Z
Learning: In `packages/components/src/ui/textarea.tsx`, the `Textarea` component forwards its ref to `FormItem`, and the textarea element receives `field.ref`, so the ref forwarding works correctly.
packages/components/src/ui/text-input.tsx (1)
Learnt from: jaruesink
PR: lambda-curry/forms#14
File: packages/components/src/ui/textarea.tsx:17-24
Timestamp: 2024-11-26T05:28:58.779Z
Learning: In `packages/components/src/ui/textarea.tsx`, the `Textarea` component forwards its ref to `FormItem`, and the textarea element receives `field.ref`, so the ref forwarding works correctly.
packages/components/src/ui/text-field.tsx (1)
Learnt from: jaruesink
PR: lambda-curry/forms#14
File: packages/components/src/ui/textarea.tsx:17-24
Timestamp: 2024-11-26T05:28:58.779Z
Learning: In `packages/components/src/ui/textarea.tsx`, the `Textarea` component forwards its ref to `FormItem`, and the textarea element receives `field.ref`, so the ref forwarding works correctly.
packages/components/src/remix-hook-form/text-field.tsx (1)
Learnt from: jaruesink
PR: lambda-curry/forms#14
File: packages/components/src/ui/textarea.tsx:17-24
Timestamp: 2024-11-26T05:28:58.779Z
Learning: In `packages/components/src/ui/textarea.tsx`, the `Textarea` component forwards its ref to `FormItem`, and the textarea element receives `field.ref`, so the ref forwarding works correctly.
🧬 Code Graph Analysis (1)
packages/components/src/remix-hook-form/text-field.tsx (1)
packages/components/src/ui/text-field.tsx (1)
TextField(67-111)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (11)
apps/docs/src/remix-hook-form/text-field.stories.tsx (2)
5-5: LGTM! Proper import for ref functionality.The
useRefimport is correctly added to support the new ref demonstration.
42-42: LGTM! Correct ref typing.The ref is properly typed as
HTMLInputElementwhich matches the underlying input element.packages/components/src/ui/text-input.tsx (3)
1-1: LGTM! Correct React import for forwardRef.The import is correctly changed from type-only to full import to support
React.forwardRef.
6-19: LGTM! Excellent forwardRef implementation.The component is correctly converted to use
React.forwardRefwith proper typing forHTMLInputElement. The ref is properly forwarded to the underlying input element.
20-20: LGTM! Good debugging practice.Adding
displayNameimproves the development experience and React DevTools identification.packages/components/src/remix-hook-form/text-field.tsx (3)
1-1: LGTM! Necessary React import.The React import is correctly added to support
React.forwardRef.
9-26: LGTM! Clean ref forwarding wrapper.The component is correctly converted to use
React.forwardRefand properly forwards the ref to the underlyingBaseTextField. The ref forwarding chain is maintained without disrupting the existing component composition logic.
28-28: LGTM! Appropriate displayName.The
RemixTextFielddisplayName clearly distinguishes this wrapper from the base TextField component.packages/components/src/ui/text-field.tsx (3)
1-1: LGTM! Required React import change.The import is correctly changed to support
React.forwardRef.
60-60: LGTM! Proper type enhancement for ref support.The
Inputcomponent type is correctly updated to acceptReact.RefAttributes<HTMLInputElement>, ensuring type safety for ref forwarding.
67-111: LGTM! Comprehensive forwardRef implementation.The component is expertly converted to use
React.forwardRefwhile preserving all existing functionality:
- Proper type parameters for
HTMLInputElementandTextInputProps- Ref correctly forwarded to the
InputComponent- All form field rendering logic maintained intact
- Supports both default
TextInputand custom input componentsThe ref forwarding chain is now complete from parent components down to the actual DOM input element.
| @@ -1,11 +1,12 @@ | |||
| import type * as React from 'react'; | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
this line could still be "type" right?
| export interface InputProps extends React.InputHTMLAttributes<HTMLInputElement> {} | ||
|
|
||
| function TextInput({ className, type, ...props }: InputProps) { | ||
| const TextInput = React.forwardRef<HTMLInputElement, InputProps>(({ className, type, ...props }, ref) => { |
There was a problem hiding this comment.
@lcmohsen instead of using forward Ref, can we follow React 19's patterns for passing in refs?
| export const TextField = React.forwardRef<HTMLInputElement, TextInputProps>( | ||
| ({ control, name, label, description, className, components, prefix, suffix, ...props }, ref) => { | ||
| // Use the custom Input component if provided, otherwise use the default TextInput |
There was a problem hiding this comment.
instead of adding forward ref, can we pass in ref as a prop for React 19 patterns?
| export type TextFieldProps = Omit<BaseTextFieldProps, 'control'>; | ||
|
|
||
| export function TextField(props: TextFieldProps) { | ||
| export const TextField = React.forwardRef<HTMLInputElement, TextFieldProps>((props, ref) => { |
There was a problem hiding this comment.
instead of using forwardRef let's use ref for React 19 patterns
- Changed imports from React to type imports for better type safety. - Refactored TextField to use a standard function instead of React.forwardRef, enhancing clarity. - Updated TextInput to support ref prop directly, ensuring consistent ref handling across components.
🔍 PR Review SummaryGreat work on adding ref support to the TextField components! The functionality works well and the storybook example clearly demonstrates the feature. However, since this project is using React 19, there are some opportunities to modernize the implementation based on React 19's new ref patterns. ✅ What's Working Well
🚀 Suggested ImprovementsBased on the feedback from @jaruesink and React 19 best practices, here are the key areas for improvement: 1. Adopt React 19 Ref PatternsSince this project uses React 19 ( 2. Optimize TypeScript ImportsKeep imports as 3. Consistent Pattern ApplicationApply the new ref pattern consistently across all components in the PR. 📋 Specific Action ItemsI'll provide detailed line-by-line feedback in the review comments, but the main changes needed are:
The React 19 approach is much cleaner - instead of wrapping components with This modernization will make the codebase more future-proof and easier to maintain! 🎯 |
![codegen-sh[bot]](https://avatars.githubusercontent.com/in/321279?s=60&v=4){kind=small}
| @@ -1,11 +1,14 @@ | |||
| import type * as React from 'react'; | |||
There was a problem hiding this comment.
You're absolutely right! This should remain as a type import for better TypeScript optimization and smaller bundle size. The type import tells TypeScript this is only used for type checking and won't be included in the runtime bundle.
import type * as React from 'react';
| } | ||
|
|
||
| function TextInput({ className, type, ...props }: InputProps) { | ||
| function TextInput({ className, type, ref, ...props }: InputProps) { |
There was a problem hiding this comment.
Perfect suggestion! With React 19, we can eliminate forwardRef entirely and use the much cleaner ref-as-prop pattern. Here's how this should look:
export interface InputProps extends React.InputHTMLAttributes<HTMLInputElement> {
ref?: React.Ref<HTMLInputElement>;
}
function TextInput({ className, type, ref, ...props }: InputProps) {
return (
<input
ref={ref}
type={type}
className={cn(
'flex h-10 w-full text-base sm:text-sm rounded-md border border-input bg-background px-3 py-2 ring-offset-background file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-foreground placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50',
className,
)}
{...props}
/>
);
}This is much cleaner and leverages React 19's native ref prop support!
|
|
||
| export const TextField = ({ | ||
| export const TextField = function TextField({ | ||
| control, |
There was a problem hiding this comment.
Exactly! Let's apply the React 19 ref-as-prop pattern here as well. This will be much cleaner:
export interface TextInputProps extends Omit<InputProps, 'prefix' | 'suffix'> {
control: Control<any>;
name: string;
label?: string;
description?: string;
components?: Partial<FieldComponents> & {
Input?: React.ComponentType<InputProps & { ref?: React.Ref<HTMLInputElement> }>;
};
prefix?: React.ReactNode;
suffix?: React.ReactNode;
className?: string;
ref?: React.Ref<HTMLInputElement>;
}
export const TextField = function TextField({
control,
name,
label,
description,
className,
components,
prefix,
suffix,
ref,
...props
}: TextInputProps) {
// ... rest of implementation
return (
<Controller
control={control}
name={name}
render={({ field, fieldState }) => (
// ... existing JSX with ref passed to InputComponent
<InputComponent
{...field}
{...props}
ref={ref}
// ... rest of props
/>
)}
/>
);
};This eliminates the need for forwardRef and makes the component much more straightforward!
| export type TextFieldProps = Omit<BaseTextFieldProps, 'control'>; | ||
|
|
||
| export function TextField(props: TextFieldProps) { | ||
| export const TextField = function RemixTextField(props: TextFieldProps & { ref?: React.Ref<HTMLInputElement> }) { |
There was a problem hiding this comment.
Perfect suggestion! Let's apply the React 19 pattern here too. The remix-hook-form TextField should also use the ref-as-prop pattern:
import type * as React from 'react';
import { TextField as BaseTextField, type TextInputProps as BaseTextFieldProps } from '../ui/text-field';
import { FormControl, FormDescription, FormLabel, FormMessage } from './form';
import { useRemixFormContext } from 'remix-hook-form';
export type TextFieldProps = Omit<BaseTextFieldProps, 'control'> & {
ref?: React.Ref<HTMLInputElement>;
};
export const TextField = function RemixTextField({ ref, ...props }: TextFieldProps) {
const { control } = useRemixFormContext();
// Merge the provided components with the default form components
const defaultComponents = {
FormControl,
FormLabel,
FormDescription,
FormMessage,
};
const components = {
...defaultComponents,
...props.components,
};
return <BaseTextField control={control} components={components} ref={ref} {...props} />;
};
TextField.displayName = 'RemixTextField';This maintains consistency with the React 19 pattern across all TextField components!
…rogrammatic focus and better integration
2 participants
Summary by CodeRabbit
New Features
Refactor