Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow shouldRequireFieldUpdates to be passed to SQFormGuidedWorkflow task modules #852

Open
20BBrown14 opened this issue Feb 21, 2023 · 4 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@20BBrown14
Copy link
Contributor

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

@20BBrown14 20BBrown14 added enhancement New feature or request question Further information is requested labels Feb 21, 2023
@laurelbean
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

aharpt commented Feb 22, 2023

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants