jedinrk / quiz_test

0 stars 0 forks source link

Code Review Comments #1

Open dkhierl-chacrasoftware opened 9 months ago

dkhierl-chacrasoftware commented 9 months ago

custom hook useMultiStepForm is a good idea but it implements anti pattern by passing an array of element to its argument and returning back an element. One better way to make this more maintainable is by using a context. here is an example of the composition.

<MultiStepForm> // here it also contains the context provider
  <UserFormEmail/> // now this form can access the state from the context and will conditionally render
</MultiStemForm>

// optional you can also make the form reusable like so
<Form
  heading="Form title"
  description="Form description"
  icon={<Star/>} // optional, take note its a type of ReactNode
  // etc 
/>
jedinrk commented 9 months ago

@dkhierl-chacrasoftware Thanks David. It took me sometime to figure out the React context to use it instead of the custom hook. But I have managed to implement it. Please review it when possible and let me know you thoughts.

dkhierl-chacrasoftware commented 9 months ago

Hi @jedinrk , Looks good! just make sure to fix typescript error. seems like you forgot to export some interface or types.

jedinrk commented 9 months ago

Thanks. Fixed it! @dkhierl-chacrasoftware

dkhierl-chacrasoftware commented 9 months ago

looks good!

dkhierl-chacrasoftware commented 9 months ago

@jedinrk sorry need to reopen, the form is not properly working

image

jedinrk commented 9 months ago

@dkhierl-chacrasoftware My bad! I had a last minute UI improvement done, which led to a missing phone number validation logic. I have fixed and pushed it.