search

SelectQuoteLabs / SQForm

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

Allow `shouldRequireFieldUpdates` to be passed to SQFormGuidedWorkflow task modules #852

Open 20BBrown14 opened 1 year ago

20BBrown14 commented 1 year ago

Currently, every task module has shouldRequireFieldUpdates set to true. We should allow consumers to pass that prop as part of the task module definition.

The main question we need to answer is if we'll want to introduce a breaking change by setting the default to false. If we default to true, that contradicts our other components which default to false

In my opinion I think setting the default to false, with a breaking change, is the route to go to keep our consistency throughout the library. Interested in other thoughts as well.

SQFormButton SQFormDialog SQFormScrollableCard

laurelbean commented 1 year ago

Most of the forms I see set shouldRequireFieldUpdates to true so maybe what we need to do is change the default to true across the board. It seems to be the more common use case. But I agree the default should be the same across components so a breaking change is needed.

hvilander commented 1 year ago

I lean in the direction of true being the default. I also understand that this will create non-trival amount of effort to migrate when ever we put this breaking change it. Consistency is good. A possible middle ground is to do the guided workflow breaking change making it consistent with current components with a plan to update all of the defaults on a different breaking change.

Since we have a lot of projects not yet past a few of the major changes maybe another big breaking change should be down the road a bit. I kinda playing devils advocate there. If the right thing is to change the default across the board I am not against it. Just putting voice to the negatives.

All that said I land slightly on the side of updating the default to true across the board.

aharpt commented 1 year ago

It seems that technically atm the default is false per the useFormButton.tsx file. I would probably second changing the default to true is that seems to be the more common use, but of course that is just me.

laurelbean commented 1 year ago

Coming back to this: I think making the change that will be easiest to consume will be the best option here. So let's go with just making the breaking change to default to false on the Guided Workflow. There aren't that many out there yet so it shouldn't be too burdensome to take in that breaking change.