feat: add invalidation for input fields #545
Conversation
|
@houssembaltii is attempting to deploy a commit to the Team Bearstudio Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request enhance error handling across multiple form components by integrating Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
![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
🧹 Outside diff range and nitpick comments (12)
src/components/Form/FieldTextarea/index.tsx (1)
46-46: Consider adding error message display.While the
isInvalidprop is correctly implemented, the error message fromfieldState.errorcould be passed to theFormFieldErrorcomponent for better user feedback.- <FormFieldError /> + <FormFieldError error={fieldState.error?.message} />src/components/Form/FieldCheckbox/index.tsx (1)
48-48: Consider adding error message display.While the
isInvalidprop is correctly implemented usingfieldState.error, I notice that althoughFormFieldErroris rendered, it's not being passed the error message fromfieldState.error.Consider passing the error message to FormFieldError:
- <FormFieldError /> + <FormFieldError error={fieldState.error?.message} />src/components/Form/FieldSelect/index.tsx (1)
Line range hint
63-67: Consider enhancing error handling and fixing type issuesWhile the current implementation works, consider these improvements:
- The nearby
@ts-expect-errorcomment indicates a typing issue that should be addressed- The error handling could be more granular by supporting different visual states for different types of errors
Consider this enhancement:
- isInvalid={!!fieldState.error} + isInvalid={!!fieldState.error} + aria-invalid={!!fieldState.error} + aria-errormessage={fieldState.error?.message} + // Remove @ts-expect-error by properly typing the onChange handler + onChange={(option: SelectOption) => onChange(option?.value)}This would:
- Improve accessibility with ARIA attributes
- Provide more detailed error feedback
- Help resolve the typing issue
src/components/Form/FieldRadios/index.tsx (1)
73-73: Consider handling specific error statesWhile the current implementation works, you might want to consider handling specific error states differently. For example, some validation errors might warrant different visual treatments.
- isInvalid={!!fieldState.error} + isInvalid={!!fieldState.error} + aria-invalid={!!fieldState.error} + aria-errormessage={fieldState.error?.message}src/components/Form/FieldMultiSelect/index.tsx (3)
42-42: Consider adding explicit type annotation for fieldState.While the current implementation works, adding explicit type annotation for
fieldStatewould improve type safety and documentation.- render={({ field: { value, onChange, ...field }, fieldState }) => { + render={({ field: { value, onChange, ...field }, fieldState: { error, invalid, isTouched, isDirty } }) => {
63-63: Consider using more explicit validation state handling.The current implementation only checks for error existence. Consider using the
invalidstate directly fromfieldStatefor more semantic code and potentially including other validation states for better user feedback.- isInvalid={!!fieldState.error} + isInvalid={fieldState.invalid} + aria-invalid={fieldState.invalid}
Fix type safety in FieldMultiSelect's onChange handler
The typing issue can be resolved by properly typing the onChange handler. The issue occurs because:
- The Select component is using chakra-react-select's types
- The onChange handler receives an array of options when isMulti is true
- The current implementation doesn't properly type the options parameter
Here's what needs to be fixed:
- The onChange handler should be typed as
(options: readonly { value: PathValue<TFieldValues, TName>[number] }[]) => void- Remove the @ts-expect-error comment once the proper typing is applied
The typing issue is legitimate and can be fixed by properly typing the onChange handler to match the chakra-react-select's type definitions while maintaining compatibility with the form field value type.
🔗 Analysis chain
Line range hint
64-68: Fix the typing issue in onChange handler.There's a type safety issue that's currently being suppressed with @ts-expect-error. This should be addressed to prevent potential runtime errors.
Let's verify the typing issue:
Would you like me to help resolve the typing issue by providing a properly typed implementation?
The previous ast-grep pattern might have been too specific. Let's try to understand:
- Where this component is used
- What type definitions exist for Select components
- The broader context of onChange handlers in select components
This will help us verify if this is a legitimate typing issue that needs fixing or if there's an existing type pattern we should follow.
Let's try without the typescript file type specification since it wasn't recognized. We'll look for:
- The component file location
- Type definitions
- Usage patterns
- Similar onChange handlers
This should help us understand the typing context and whether this is a common pattern or an isolated issue.
From the previous results, we found the relevant files but need to see their contents to understand:
- The Select component's type definitions that are being referenced
- The actual implementation of FieldMultiSelect to understand the typing issue
- The relationship between SelectProps and the onChange handler
This will help us determine if this is a legitimate typing issue and how to properly fix it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns in other select components to understand the correct typing ast-grep --pattern 'onChange={(options) => $$$}'Length of output: 50
Script:
#!/bin/bash # Let's try a broader search for onChange handlers in select components rg -l "onChange.*Select" --type typescript # Then look for type definitions related to Select rg "type.*Select" --type typescript # Also search for the specific component usage rg "FieldMultiSelect" --type typescript -A 5 -B 5Length of output: 237
Script:
#!/bin/bash # Let's try without typescript file type specification # Search for the component file first fd "FieldMultiSelect" # Then look for Select-related type definitions rg "type.*Select" # And search for usage patterns rg "FieldMultiSelect" -A 5 -B 5 # Look for similar onChange patterns rg "onChange.*Select" -A 2 -B 2Length of output: 9543
Script:
#!/bin/bash # Let's look at the Select component's type definitions cat src/components/Select/index.tsx # And examine the FieldMultiSelect implementation cat src/components/Form/FieldMultiSelect/index.tsxLength of output: 5041
src/components/Form/FieldCheckboxes/index.tsx (1)
73-73: Consider enhancing validation feedback and accessibilityWhile the basic error state is implemented correctly, consider these improvements:
<Checkbox key={option.value} value={option.value} - isInvalid={!!fieldState.error} + isInvalid={fieldState.invalid && fieldState.touched} + aria-invalid={fieldState.invalid} + aria-errormessage={fieldState.error?.message} {...props.checkboxProps} >This would:
- Only show validation after the field is touched
- Improve screen reader support with ARIA attributes
- Provide more detailed error feedback
src/components/Form/FieldPassword/index.tsx (1)
Line range hint
47-82: Consider additional security measures for password fieldWhile the implementation is functional, consider adding these security enhancements:
- Add autocomplete="new-password" to prevent password autofill in registration forms
- Consider adding a password strength indicator
- Add data-testid attributes for testing sensitive functionality
Example implementation:
<Input isDisabled={props.isDisabled} type={showPassword ? 'text' : 'password'} placeholder={props.placeholder} autoFocus={props.autoFocus} isInvalid={!!fieldState.error} + autoComplete="new-password" + data-testid="password-input" {...props.inputProps} {...field} />src/components/Form/FieldCustom/docs.stories.tsx (3)
47-54: LGTM! Consider applying similar changes to other stories.The implementation correctly integrates field invalidation using
fieldState.error. However, the same error handling pattern should be applied to the DefaultValue and Disabled stories for consistency.Apply this pattern to the other stories:
// In DefaultValue story -render={({ field }) => ( +render={({ field, fieldState }) => ( <> - <Input w={24} size="sm" {...field} /> + <Input w={24} size="sm" {...field} isInvalid={!!fieldState.error} /> <FormFieldError /> </> )} // In Disabled story -render={({ field }) => ( +render={({ field, fieldState }) => ( <> - <Input isDisabled w={24} size="sm" {...field} /> + <Input isDisabled w={24} size="sm" {...field} isInvalid={!!fieldState.error} /> <FormFieldError /> </> )}
Line range hint
19-23: Consider enhancing validation feedback in the UI.The schema enforces an 8-character requirement, but this constraint isn't visible to users until validation fails.
Consider adding helper text to improve user experience:
<FormFieldController control={form.control} name="other" type="custom" render={({ field, fieldState }) => ( <> <Input w={24} size="sm" {...field} isInvalid={!!fieldState.error} /> + <Text fontSize="sm" color="text-dimmed"> + {field.value?.length ?? 0}/8 characters + </Text> <FormFieldError /> </> )} />
Line range hint
41-63: Consider improving error message layout.The current Flex layout might not handle error messages optimally, especially with the "Before/After" text alignment.
Consider restructuring the layout to better accommodate error messages:
-<Flex align="center" gap={4}> +<Stack spacing={2}> + <Flex align="center" gap={4}> <FormFieldLabel>Label</FormFieldLabel> <Text color="text-dimmed" fontSize="sm"> Before </Text> <FormFieldController control={form.control} name="other" type="custom" render={({ field, fieldState }) => ( <> <Input w={24} size="sm" {...field} isInvalid={!!fieldState.error} /> - <FormFieldError /> </> )} /> <Text color="text-dimmed" fontSize="sm"> After </Text> + </Flex> + <FormFieldError /> +</Stack> -</Flex>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/components/Form/FieldCheckbox/index.tsx(1 hunks)src/components/Form/FieldCheckboxes/index.tsx(2 hunks)src/components/Form/FieldCustom/docs.stories.tsx(1 hunks)src/components/Form/FieldMultiSelect/index.tsx(2 hunks)src/components/Form/FieldPassword/index.tsx(1 hunks)src/components/Form/FieldRadios/index.tsx(2 hunks)src/components/Form/FieldSelect/index.tsx(2 hunks)src/components/Form/FieldTextarea/index.tsx(1 hunks)
🔇 Additional comments (9)
src/components/Form/FieldTextarea/index.tsx (1)
38-38: LGTM! Good use of fieldState for validation.
The addition of fieldState to the render props is a clean way to access the field's validation state.
src/components/Form/FieldCheckbox/index.tsx (1)
42-42: LGTM: Proper integration of fieldState from react-hook-form.
The addition of fieldState to the destructured props from Controller's render function is correct and follows react-hook-form's best practices for accessing field validation state.
src/components/Form/FieldSelect/index.tsx (3)
44-44: LGTM: Proper integration of fieldState
The addition of fieldState parameter follows react-hook-form's best practices for accessing field validation state.
63-63: LGTM: Proper implementation of error state
The isInvalid prop is correctly implemented using the field's error state, and the double negation ensures a boolean value.
Line range hint 44-67: Verify consistent implementation across form components
The AI summary indicates this pattern is being applied to multiple form components. Let's verify the consistency of the implementation.
✅ Verification successful
Let me try a different approach to verify the implementation across form components.
Implementation is consistent across form field components
The verification shows consistent implementation patterns across all form field components:
- All components use the
render={({ field, fieldState })pattern with proper destructuring - Error handling is uniform with
isInvalid={!!fieldState.error}across all field types - Components follow the same structure with
<Flex>container and consistent prop handling
The implementation in FieldSelect aligns perfectly with the established patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all form field components follow the same pattern
# Test: Search for all form field components and their error handling implementation
echo "Checking form field components for consistent error handling..."
rg -l "isInvalid.*fieldState.error" "src/components/Form/Field*/"
# Test: Verify consistent Controller render prop usage
echo "Checking Controller render prop implementation..."
ast-grep --pattern 'render={({ field, fieldState })'
Length of output: 436
Script:
#!/bin/bash
# Search for form field components and their implementations
echo "Searching for form field components..."
fd "Field.*\.tsx?" src/components/Form -t f
echo "\nChecking implementation patterns..."
# Look for Controller render prop pattern
rg -A 5 "render=\{.*field.*fieldState" src/components/Form
# Look for isInvalid prop usage
echo "\nChecking error handling..."
rg "isInvalid.*fieldState\.error" src/components/Form
Length of output: 8567
src/components/Form/FieldRadios/index.tsx (1)
55-55: LGTM: Proper integration of react-hook-form's fieldState
The addition of fieldState to the render props follows react-hook-form's best practices for form validation.
src/components/Form/FieldMultiSelect/index.tsx (1)
Line range hint 42-70: Implementation successfully adds field invalidation.
The changes effectively implement field invalidation by integrating fieldState and adding visual feedback through the isInvalid prop. The implementation is solid, with the suggested improvements being refinements rather than critical issues.
src/components/Form/FieldCheckboxes/index.tsx (1)
55-55: LGTM: Clean integration of fieldState
The addition of fieldState from react-hook-form's Controller is well implemented and follows the library's best practices.
src/components/Form/FieldPassword/index.tsx (1)
47-47: LGTM: Proper integration of fieldState
The addition of fieldState parameter follows React Hook Form's best practices for accessing field metadata.
| <Flex flexDirection="column" gap={1} flex={1} {...props.containerProps}> | ||
| <InputGroup size={props.size}> | ||
| <Input | ||
| isDisabled={props.isDisabled} | ||
| type={showPassword ? 'text' : 'password'} | ||
| placeholder={props.placeholder} | ||
| autoFocus={props.autoFocus} | ||
| isInvalid={!!fieldState.error} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass error details to FormFieldError component
While the isInvalid prop is correctly implemented, the FormFieldError component should receive the error details to display the actual error message.
Apply this change:
<InputGroup size={props.size}>
<Input
isDisabled={props.isDisabled}
type={showPassword ? 'text' : 'password'}
placeholder={props.placeholder}
autoFocus={props.autoFocus}
isInvalid={!!fieldState.error}
{...props.inputProps}
{...field}
/>
{/* ... other elements ... */}
</InputGroup>
- <FormFieldError />
+ <FormFieldError error={fieldState.error} />Also applies to: 82-82
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Describe your changes
Add invalidation for :
Screenshots
FieldCheckbox
FieldCheckboxes
FieldCustom
FIeldMultiSelect
FieldPassword
FieldRadios
FieldSelect
FieldTextarea
Documentation
Checklist
pnpm storybookcommand and everything is working