search

kettanaito / react-advanced-form

Functional reactive forms. Multi-layer validation, custom styling, field grouping, reactive props, and much more.
https://redd.gitbook.io/react-advanced-form
MIT License
217 stars 24 forks source link

Unexpected `onFirstChange` behavior with regard to `Form.reset()` #371

Closed kayleecodes1 closed 5 years ago

kayleecodes1 commented 5 years ago

Environment

What

Current behavior

Expected behavior

I would personally expect a form that has been reset to no longer be dirty.

My use case is a user profile form. It is only submittable when the form is dirty. After it is successfully submitted and updated via the server I call Form.setValues() and Form.reset(). Currently I can't figure out a way to track its dirty state after past point.

Why

The only place the Form's dirty state property is ever set to false is in the constructor. https://github.com/kettanaito/react-advanced-form/blob/master/src/components/Form.jsx

It seems like either a design decision or an oversight. If it's intentional I'm just looking for a way to handle my use case. Otherwise I'm happy to submit a PR to update this behavior.

How

Here is a CodeSandbox demo: https://codesandbox.io/s/k54v4r9mvo

kayleecodes1 commented 5 years ago

Not the same issue but related to my use case: how do the maintainers feel about an optional initialValues parameter when calling Form.reset(), to change the values to which it resets?

kettanaito commented 5 years ago

Thanks for submitting such detailed issue, @kylepixel!

You are right about this behavior being an oversight. Form's dirty state is intended to be reset to false after form is reset.

https://github.com/kettanaito/react-advanced-form/blob/288fe08ca357f170534fd16606b73defc414e5c9/src/components/Form.jsx#L604

Here we would need to also add dirty: false alongside the next fields value.

May I kindly ask you to provide a pull request for this? It should be quite trivial: editing that setState call and ideally providing an integration test that ensures this behavior. You can migrate your sandbox example to the repository to be used as an integration test scenario, it looks solid!

kettanaito commented 5 years ago

Regarding optional initialValues, I believe this is unnecessary. Reset is meant to reset the field state and values to the initial state (and initial values). This means that when you render the following field:

<Input name="foo" initialValue="bar" />

and call form.reset() you expect the field's value to become "bar".

Regarding your use case, reseting a field to a specific value is really just setting the value of a field. So you can use form.setValues() for that purpose, including its combination with the reset:

form.reset()
form.setValues({ foo: 'custom value' })

I believe those actions are Promise-based, so you may want to chain them properly.

kayleecodes1 commented 5 years ago

Thanks for the quick response!

I'll open a PR today for the fix to reset().

And your response regarding initialValues makes sense. Your proposed solution will work for me.

kettanaito commented 5 years ago

Fixed in #373. Will be published in the next patch version.