Uncontrolled and controlled input #6
Conversation
Added to test if it's gonna work the integration with it
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 60.44% 67.68% +7.23%
==========================================
Files 5 5
Lines 134 164 +30
Branches 21 28 +7
==========================================
+ Hits 81 111 +30
Misses 49 49
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/InputClickEdit/InputClickEdit.tsx (4)
29-32: Consider improving type safety for HTML input attributes.The current Omit usage is good, but we can make it more type-safe by explicitly typing the allowed input types.
- } & Omit< - React.InputHTMLAttributes<HTMLInputElement>, - "value" | "defaultValue" | "onChange" | "type" - >; + } & Omit< + Pick< + React.InputHTMLAttributes<HTMLInputElement>, + | "placeholder" + | "disabled" + | "required" + | "autoFocus" + | "name" + | "id" + | "aria-label" + | "aria-describedby" + >, + "value" | "defaultValue" | "onChange" | "type" + >;
69-73: Consider the necessity of isControlled in the dependency array.The
isControlledvalue is derived fromvalue !== undefinedand won't change during the component's lifecycle unlessvaluechanges. Sincevalueis already in the dependency array, includingisControlledis redundant.useEffect(() => { if (isControlled) { setInternalValue(value); } - }, [value, isControlled]); + }, [value]);
80-86: Add explicit type for the onChange event handler.Consider adding an explicit type for better type safety and documentation.
- const onChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const onChange: React.ChangeEventHandler<HTMLInputElement> = (e) => { const newValue = e.target.value; if (!isControlled) { setInternalValue(newValue); } onInputChange?.(newValue); };
121-129: Enhance button accessibility with role and type attributes.The buttons should have explicit type attributes to prevent form submission, and the role attribute could be more specific.
<button data-testid="action-button" className={cn(buttonBaseClassName, saveButtonClassName)} onClick={handleSave} + type="button" + role="switch" + aria-pressed="true" aria-label={iconsOnly ? saveButtonLabel?.toString() : undefined} >src/InputClickEdit/InputClickEdit.test.tsx (1)
150-161: Add edge cases to controlled mode tests.Consider adding tests for the following scenarios:
- Handling undefined/null values
- Empty string values
- Special characters
it("should handle undefined and null values gracefully", () => { const { rerender } = render(<InputClickEdit value={undefined} isEditing />); expect(screen.getByRole("textbox")).toHaveValue(""); rerender(<InputClickEdit value={null} isEditing />); expect(screen.getByRole("textbox")).toHaveValue(""); }); it("should handle empty string values", () => { render(<InputClickEdit value="" isEditing />); expect(screen.getByRole("textbox")).toHaveValue(""); }); it("should handle special characters", () => { const specialChars = "!@#$%^&*()_+"; render(<InputClickEdit value={specialChars} isEditing />); expect(screen.getByRole("textbox")).toHaveValue(specialChars); });README.md (1)
156-180: Enhance React Hook Form integration documentation.The current example is good but could be more comprehensive. Consider adding:
- Error handling examples
- Form validation examples
- Multiple fields integration
// Add these examples to the documentation: // 1. With validation function FormWithValidation() { const { control } = useForm({ defaultValues: { name: "" }, rules: { name: { required: "Name is required" } } }); return ( <Controller name="name" control={control} render={({ field, fieldState: { error } }) => ( <div> <InputClickEdit {...field} onInputChange={field.onChange} /> {error && <span className="error">{error.message}</span>} </div> )} /> ); } // 2. Multiple fields function MultipleFields() { const { control } = useForm({ defaultValues: { firstName: "", lastName: "" } }); return ( <> <Controller name="firstName" control={control} render={({ field }) => ( <InputClickEdit {...field} label="First Name" onInputChange={field.onChange} /> )} /> <Controller name="lastName" control={control} render={({ field }) => ( <InputClickEdit {...field} label="Last Name" onInputChange={field.onChange} /> )} /> </> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
README.md(3 hunks)package.json(1 hunks)src/InputClickEdit/InputClickEdit.test.tsx(2 hunks)src/InputClickEdit/InputClickEdit.tsx(3 hunks)
| "jsdom": "^26.0.0", | ||
| "lint-staged": "^15.3.0", | ||
| "prettier": "^3.4.2", | ||
| "react-hook-form": "^7.54.2", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move react-hook-form to peerDependencies.
Since react-hook-form is an optional integration feature, it should be a peer dependency rather than a dev dependency. This allows users to choose whether to install it and which version to use.
- "react-hook-form": "^7.54.2",Add to peerDependencies:
"peerDependencies": {
"react": ">=18.0.0",
- "react-dom": ">=18.0.0"
+ "react-dom": ">=18.0.0",
+ "react-hook-form": ">=7.0.0"
},📝 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.
| "react-hook-form": "^7.54.2", | |
| "peerDependencies": { | |
| "react": ">=18.0.0", | |
| "react-dom": ">=18.0.0", | |
| "react-hook-form": ">=7.0.0" | |
| }, |
There was a problem hiding this comment.
But I need this as a devDependency because I am using in one test
There was a problem hiding this comment.
@matheusrocha89 You're right! Let's keep it in devDependencies for testing, but also add it to peerDependencies since it's an optional integration. This is a common pattern where a package is both a peer and dev dependency:
"peerDependencies": {
"react": ">=18.0.0",
- "react-dom": ">=18.0.0"
+ "react-dom": ">=18.0.0",
+ "react-hook-form": ">=7.0.0"
},
"devDependencies": {
// ... other dev dependencies ...
"react-hook-form": "^7.54.2",
}✏️ Learnings added
Learnt from: matheusrocha89
PR: matheusrocha89/react-click-edit#6
File: package.json:72-72
Timestamp: 2025-01-27T09:09:26.505Z
Learning: When a package is used for optional integration features and also needed for testing, it should be listed in both peerDependencies (with a more relaxed version range) and devDependencies (with a specific version for testing).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
renaming properties to keep the same name as the input props. This includes changing some types too BREAKING CHANGE: onInputChange is now onChange
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/InputClickEdit/InputClickEdit.tsx (3)
54-66: Optimize the initial state management.While the implementation correctly handles both controlled and uncontrolled modes, the initial state setup can be improved to avoid unnecessary re-renders.
Consider this optimization:
-const [internalValue, setInternalValue] = useState(value ?? defaultValue); +const [internalValue, setInternalValue] = useState(() => value ?? defaultValue);Using the function form of useState prevents re-computation of the initial state on re-renders.
68-84: Consider memoizing event handlers.While the handlers are implemented correctly, they could be memoized to prevent unnecessary re-renders of child components.
Consider this optimization:
+import { useCallback } from "react"; -const onEditClick = () => { +const onEditClick = useCallback(() => { setEditing(true); onEditButtonClick?.(); -}; +}, [onEditButtonClick]); -const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { +const handleChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { const newValue = e.target.value; if (!isControlled) { setInternalValue(newValue); } onChange?.(e); -}; +}, [isControlled, onChange]); -const handleSave = () => { +const handleSave = useCallback(() => { setEditing(false); onSaveButtonClick?.(); -}; +}, [onSaveButtonClick]);
102-139: Enhance button accessibility.While the buttons have aria-labels for icon-only mode, they could benefit from additional accessibility attributes.
Consider these accessibility enhancements:
<button data-testid="action-button" className={cn(buttonBaseClassName, saveButtonClassName)} onClick={handleSave} aria-label={iconsOnly ? saveButtonLabel?.toString() : undefined} + type="button" + aria-pressed={editing} > {(showIcons || iconsOnly) && <SaveIcon data-testid="save-icon" />} {!iconsOnly && saveButtonLabel} </button><button data-testid="action-button" className={cn(buttonBaseClassName, editButtonClassName)} onClick={onEditClick} aria-label={iconsOnly ? editButtonLabel?.toString() : undefined} + type="button" + aria-pressed={!editing} > {(showIcons || iconsOnly) && <EditIcon data-testid="edit-icon" />} {!iconsOnly && editButtonLabel} </button>src/InputClickEdit/InputClickEdit.test.tsx (2)
58-67: Enhance onChange test coverage.While the test correctly verifies the event handling, it could be more comprehensive by testing both controlled and uncontrolled scenarios.
Consider adding these test cases:
it('should maintain controlled value on change', () => { const onChange = vi.fn(); render(<InputClickEdit isEditing value="Controlled" onChange={onChange} />); fireEvent.change(screen.getByRole("textbox"), { target: { value: "New Value" } }); expect(screen.getByRole("textbox")).toHaveValue("Controlled"); expect(onChange).toHaveBeenCalled(); }); it('should update uncontrolled value on change', () => { const onChange = vi.fn(); render(<InputClickEdit isEditing defaultValue="Uncontrolled" onChange={onChange} />); fireEvent.change(screen.getByRole("textbox"), { target: { value: "New Value" } }); expect(screen.getByRole("textbox")).toHaveValue("New Value"); expect(onChange).toHaveBeenCalled(); });
182-209: Add error scenario tests for form integration.While the happy path is well tested, consider adding tests for error scenarios.
Consider adding these test cases:
it('should handle form validation errors', async () => { const TestFormWithValidation = () => { const { register, formState: { errors } } = useForm({ defaultValues: { test: '' }, }); return ( <form> <InputClickEdit {...register('test', { required: 'This field is required' })} isEditing /> {errors.test && <span role="alert">{errors.test.message}</span>} </form> ); }; render(<TestFormWithValidation />); const input = screen.getByRole('textbox'); fireEvent.blur(input); await waitFor(() => { expect(screen.getByRole('alert')).toHaveTextContent('This field is required'); }); });README.md (2)
47-65: Enhance props documentation.While the props table is comprehensive, it could be improved with additional details.
Consider adding:
- Required/optional status for each prop
- Default value column for all props
- Type examples for complex props
- Cross-references to relevant examples
Example enhancement:
| Prop | Type | Required | Default | Description | | ----------------- | ------------------------------------ | -------- | -------- | ----------------------------------------- | | value | string | No | - | Controlled text value to display and edit. See [Controlled Example](#controlled-example) | | defaultValue | string | No | - | Initial uncontrolled value. See [React Hook Form Integration](#react-hook-form-integration) |
152-195: Enhance form integration examples.While the examples demonstrate basic integration, they could be more comprehensive.
Consider adding:
- Form validation examples
- Error handling examples
- Form submission handling
- TypeScript type annotations
Example enhancement:
import { useForm, Controller } from 'react-hook-form'; import { InputClickEdit } from '@nobrainers/react-click-edit'; interface FormData { editableText: string; } function FormExample() { const { control, handleSubmit, formState: { errors } } = useForm<FormData>({ defaultValues: { editableText: 'Edit me' } }); const onSubmit = (data: FormData) => console.log(data); return ( <form onSubmit={handleSubmit(onSubmit)}> <Controller name="editableText" control={control} rules={{ required: 'This field is required' }} render={({ field, fieldState: { error } }) => ( <> <InputClickEdit {...field} onChange={field.onChange} /> {error && <span className="error">{error.message}</span>} </> )} /> <button type="submit">Submit</button> </form> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(5 hunks)src/InputClickEdit/InputClickEdit.test.tsx(4 hunks)src/InputClickEdit/InputClickEdit.tsx(2 hunks)
🔇 Additional comments (2)
src/InputClickEdit/InputClickEdit.tsx (1)
25-53: LGTM! Well-structured component initialization.The component follows React best practices by:
- Extending native input attributes
- Using forwardRef for proper ref handling
- Following React's naming conventions
src/InputClickEdit/InputClickEdit.test.tsx (1)
154-180: LGTM! Comprehensive controlled/uncontrolled mode testing.The test suites thoroughly verify both controlled and uncontrolled behaviors, including:
- Value propagation in controlled mode
- Internal state management in uncontrolled mode
- Initial value handling
also improving accessibility
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/InputClickEdit/InputClickEdit.tsx (3)
54-68: Consider adding cleanup to useEffect.While the state management is well-implemented, consider adding a cleanup function to prevent state updates on unmounted component.
useEffect(() => { + let mounted = true; if (isControlled) { - setInternalValue(value); + if (mounted) { + setInternalValue(value); + } } + return () => { + mounted = false; + }; }, [value, isControlled]);
91-106: Add type safety to buttonBaseClassName object.Consider adding type safety to the className object to prevent potential runtime errors.
- const buttonBaseClassName = { + const buttonBaseClassName: Record<string, boolean> = { [styles.button]: true, [styles.buttonReverse]: iconPosition === "right", };
107-148: Add aria-label to input for better accessibility.When a label is present, consider adding an aria-label to the input for better screen reader support.
- <input {...inputProps} /> + <input {...inputProps} aria-label={label} />README.md (1)
152-195: Consider adding form validation example.The React Hook Form integration examples are good, but consider adding an example demonstrating form validation to showcase the full capabilities of the integration.
function ValidationExample() { const { control, handleSubmit, formState: { errors } } = useForm({ defaultValues: { editableText: "Edit me" } }); return ( <form onSubmit={handleSubmit(console.log)}> <Controller name="editableText" control={control} rules={{ required: "This field is required" }} render={({ field }) => ( <> <InputClickEdit {...field} /> {errors.editableText && ( <span>{errors.editableText.message}</span> )} </> )} /> <button type="submit">Submit</button> </form> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(5 hunks)src/InputClickEdit/InputClickEdit.tsx(2 hunks)
🔇 Additional comments (4)
src/InputClickEdit/InputClickEdit.tsx (2)
25-53: LGTM! Props and component initialization follow React best practices.The changes align well with React conventions:
- Props extend HTMLInputAttributes for complete coverage
- forwardRef enables proper ref handling
- Prop names follow React conventions (type, onChange)
70-89: LGTM! Event handlers are well-implemented.The implementation follows React best practices:
- Proper memoization with useCallback
- Correct controlled/uncontrolled state handling
- Standard event handling patterns
README.md (2)
47-65: LGTM! Props documentation is clear and comprehensive.The props table effectively documents:
- Updated prop names and types
- New defaultValue prop
- Clear distinction between controlled and uncontrolled modes
Line range hint
71-151: LGTM! Examples are clear and cover common use cases.The examples effectively demonstrate:
- Basic usage
- Different input types
- Custom styling
- Controlled state management
This PR adds the feature of whether the component controlled or not and can be integrated easily with react-hook-form.
Summary by CodeRabbit
Release Notes
New Features
defaultValueDocumentation
Dependencies
react-hook-formlibrary for form managementTesting