Multi-step form [React, Typescript, React Hook Form, CSS Modules]

Community feedback

• 

  T

  Alex•2,010

  @AlexKMarshallPosted 12 months ago

  This is looking very good. Functionally it all works well. I love the whimsical but still very clear focus states, and the keyboard navigation on the custom radio group is all good

  You've said styling wasn't your focus so I won't spend too long on it, but when I load the site (or hard refresh) I get some layout shift (the content seems to jump up twice before settling on its final location). My guess is this could be caused by the useMobile hook - useEffect runs after paint, so could cause jumps if any layout depends on it. You could try replacing it with a useLayoutEffect which runs before paint. Even better would be to avoid this hook completely and do things entirely in CSS, that'll remove any possibility of the wrong value being computed. I get a similar layout shift on the confirmation page too, though here I think the image not having enough reserved space may have been the issue.

  In terms of the typescript this looks ok, there's just a few comments I'd make:

  • Rely on inference where possible, it's less to type and avoids the possibility of lying to the compiler. Some particular cases of this I saw were in array map functions. You shouldn't need to add types to the callback, they should already be inferred. If they aren't, it's because the original array hasn't been typed correctly, and that's where the fix should be
  • Keep types close to where they are used. This is a bit more of a preference thing, but having a types file or folder, particularly for types that are only used in a single place, hides things too much in my opinion. If you need to declare a type, declare it inline, or directly above the function that needs it. If you need to use it somewhere else too, export it from that file. Reserve a global types folder/file for truly generic type utilities or something that really isn't related to anything specific. I would expect there to be few of these.

  For the React code there isn't anything wrong here as such. However, for my tastes the code has been abstracted too much. The result is some components that handle many different things, and as a result need to receive 10-20 props. This has a two-fold effect. First, I find it makes the code quite hard to reason about. It also makes the components very coupled. If you want to change something in this code (maybe add a new page to the form) that change will have to touch many many files. And will involve re-testing the existing pages.

  It's hard to give a small piece of advice here, and as this works you may want to ignore this anyway. But, I'll try and put together a few thoughts:

  • Avoid too many props. Props aren't bad, but, if you're finding you have to have 5, 10, even 20 props to a component, especially if some of those props seem unrelated to each other, I think that's an indication that using composition would be preferable
  • Keep hooks near the components that use them. This is the same as my advice for the types. Things like the useCurrentStep hook - this is very coupled to the app component. It can live in the same file, that way there's less jumping around the code-base when trying to figure out what a particular component does. You may find you don't even need it to be a custom hook at all.
  • Don't make components do too much. The UniqueSteps component is too complex for my tastes. It tries to handle every page of the form and the confirmation page. Instead, I would prefer to keep the step logic in app, and render a separate component for each step. Those components can then be responsible for their own instances of useForm and encapsulate all their form logic, only exposing an onSubmit and initialValues to app which can manage the global form state (but none of the validation)
  • Be careful with data-driven ui. I like the idea behind the data.json. This could be a really neat way to have forms that are customisable by content editors from a CMS. However the current implementation doesn't really go that far. While all the data is in a json file, the UniqeSteps and other components still have to know about what's in that json file. In my opinion this has now split the places that I need to read to understand the logic, without providing benefit. Personally, I would delete the data file and manually write the jsx out in each form component. Long jsx is preferable to logic that is too spread-out.

  I hope some of this is helpful and not too vague. If you've got any questions or want me to go a bit deeper on any of this I'd be happy to.

  Marked as helpful

  0

  ---

  

  T

  Alex•2,010

  @AlexKMarshallPosted 12 months ago

  To follow up on this, here are 3 articles I think should be helpful. They cover most of what I was trying to get across here:

  https://kentcdodds.com/blog/when-to-break-up-a-component-into-multiple-components

  https://kentcdodds.com/blog/aha-programming

  https://epicreact.dev/one-react-mistake-thats-slowing-you-down/

  Marked as helpful

  0