SelectQuoteLabs / SQForm

Internal Form library utilizing Formik, Yup and Material-UI
MIT License
13 stars 10 forks source link

validateOnChange and validateOnBlur should be exposed props #282

Open 20BBrown14 opened 3 years ago

20BBrown14 commented 3 years ago

Affected Components: SQForm, SQFormDialog, SQFormDialogStepper

Formik allows us to specify when we want to validate. SQForm already accepts validateOnMount. I propose one of two things.

  1. SQForm and friends should also accept validateOnChange1 and validateOnBlur2 to allow consumers to have complete control over when validation happens.

  2. Any extra props that SQForm and friends receive can be sent to Formik directly like we do with some of our components that wrap MUI components

I prefer option 2 for two reasons. One, so that we don't have to come back here and make changes to your code base if we want to add something or expose a thing that Formik exposes, and two to avoid having a mountain of props that people will have to sort through in documentation and what not to find what they need.

SeanGroff commented 3 years ago

@20BBrown14 Can you describe the use case or an example of needing to disable validateOnChange and/or validateOnBlur? These are already enabled by default.

20BBrown14 commented 3 years ago

That is true and I didn't realize that. I originally opened this because I thought it was disabled by default and I wanted to enable it. BUT since it already is enabled there must be something else going on with my work.

To answer your question I can't think of a use case, right now, to disable to them. So we could use this issue to allow any props to be passed to formik or we could just close this and reevaluate if a need is raised.

SeanGroff commented 3 years ago

That is true and I didn't realized that. I orginally opened this because I thought it was disabled by default and I wanted to enable it. BUT since it already is enabled there must be something else going on with my work.

To answer your question I can't think of a use case, right now, to disable to them. So we could use this issue to allow any props to be passed to formik or we could just close this and reevaluate if a need is raised.

Gotcha. Let's leave this issue open until you are able to identify the root cause of your issue. We generally want to avoid the {...rest} API as much as possible. Ideally, our shared components cover the 80% use case to maintain a small easy to use props API. Create a variant component to work for the rare use case(s), the less than 20%.

Creating guard rails to our props API helps maintain consistent UI/UX. If a component is too restrictive, then discussion occurs here and we can update a components props API. I'm not opposed to using the rest param (opening the floodgates) as long as we've exhausted all other options first.