-
Notifications
You must be signed in to change notification settings - Fork 0
Add react hook form to all form component. #124
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for smart-cv-migracode ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
inkpot-monkey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AferePreciousOnome sorry it has taken me so long to leave a review on this.
First off, nice work implementing React Hook Form! This is a big step up from manually handling all the form state.
However I have been thinking about this for the last few days and I think we can do better :) Right now, each section of the form (personal info, professional summary, etc.) is its own separate mini-form with its own logic. This works, but it means we're repeating a lot of code across components.
I think we should make this simpler by treating the whole multi-step form as a single form, just split across different screens. Each component then would only be responsible for its individual inputs.
This would look like:
- One main form at the top level with all the data
- Each step just renders the fields for that section
- All data lives in one place (no more passing values up and down)
The main benefit is that we would need less code and have fewer headaches with validation and keeping data in sync.
Here's how it would work:
- The main form:
Wrap everything in a FormProvider
// In your main form component
const methods = useForm({
mode: 'onBlur', // Validates fields when users tab away
defaultValues: savedData
});
<FormProvider {...methods}>
<form>
{/* Show different sections based on current step */}
{currentStep === 'PERSONAL INFO' && <PersonalInfo />}
{currentStep === 'PROFESSIONAL SUMMARY' && <ProfessionalSummary />}
</form>
</FormProvider>- Each section:
// Inside PersonalInfo.jsx
function PersonalInfo() {
// Connect to the parent form - no separate useForm needed!
const { register, formState } = useFormContext();
return (
<>
<input {...register('personalInfo.name')} />
{formState.errors.personalInfo?.name && (
<p className="error">{formState.errors.personalInfo.name.message}</p>
)}
</>
);
}For validating each step before continuing, we can use trigger and remove some more code there as well
- Auto-saving would happen at the top level:
// In your main form component
const formValues = methods.watch(); // Gets all form values
useEffect(() => {
// Save data whenever it changes (maybe with a small delay)
const timer = setTimeout(() => saveData(formValues), 500);
return () => clearTimeout(timer);
}, [formValues]);This approach means:
- No need for manual blur handlers in each component
- All form data stays in one place
- Validation works across the entire form (meaning we dont have to break up the validation)
Another thing that we can improve easily here is that we are duplicating the schema which just means maintenance will get harder and more error-prone.
I know it seems tricky because the netlify functions and the frontend are in differnet folders are last time we tried to import it everything broke. But as a fix for that try making a symlink from netlify/utils/schemaValidation.js to the frontend code folder in src/utils
|
Let me know if you have any questions about this |
…ntralized validation schema
inkpot-monkey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AferePreciousOnome nice work we are getting some where.
However I think there is still some work that we can do to make this fit more within the mental model of React Form. Using the FormProvider from React Form means that we shouldnt have to pass anything into the separate form components and instead we can get everything from the form context thus making it easier to manage.
The onChange handlers for the separate components should be combined into the central one with the watched values (though I left a comment there that we could explore using a general onBlur function to improve the performance of the application.
I have left more comments in the code. Let me know if you have any questions or comments about them
|
Lets also separate the api key from the main form data when we save to local storage as these are separate data entities. Perhaps save it as a new |
Refactored multi-step form to use a single useForm instance with FormProvider
Replaced individual form logic in each step with useFormContext for shared state access
Centralized validation across steps, removing redundant validation logic
Implemented auto-saving at the top-level using watch and useEffect
Updated savedData to use a local storage utility in src/utils
Created a symlink from Netlify backend schema to frontend for consistent validation
Reduced code duplication and improved maintainability across form steps