CodeHubOrg / codehub-mentorships

CodeHub Mentorship platform - built with Laravel and React
3 stars 0 forks source link

Katja/form improvements #37

Closed katjad closed 4 years ago

katjad commented 4 years ago

Most of this is Roopa's pull request, through which I realised that something in my form was not quite working as intended! To make the form fully generic, I'm passing the initial values in as a prop now - I'm also setting the state from that which I think is a bit of an antipattern, but might be okay in this case? Improved updateState can then be used for setting the state of multiple fields.

(also updated node modules, and removed the vue template compiler)

NeverFearTheasHere commented 4 years ago

Passing some initial values in as a prop which you use to set the initial state isn't an anti-pattern - it's quite common practice and generally regarded as a good way to solve this problem 😄

The anti-pattern which you might be thinking of is when you end up with two different "sources of truth" for the value - one being the the prop and one being the state. E.g. if you were expecting the prop value to change and this to make the component state update when it re-renders (it won't update the state!)

What you've done is good because it's clear from its name ("initial") that this prop will only be used to setup the initial state values, and after that the Form component's state will be the single source of truth :+1:

katjad commented 4 years ago

Thanks both for the reviews! And thanks for all the comments @NeverFearTheasHere. Sounds good about the generic parameter, just not something I am very familiar with yet! And glad setting state from props not an antipattern here; I'll merge in for now, as otherwise it will need approval again (have learned that today)