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

onChange called when fields are being focused/blurred #271

Closed
cmtt opened this issue Mar 7, 2018 · 16 comments
Closed

onChange called when fields are being focused/blurred #271

cmtt opened this issue Mar 7, 2018 · 16 comments

Comments

@cmtt
Copy link

cmtt commented Mar 7, 2018

Hi, thanks for all the effort and quick updates.

I noticed an issue related to change detection: when a user focuses a field and blurs it, onChange is called. The change detection can be found in src/components/Form.js:

  componentWillReceiveProps (nextProps) {
    const didUpdate = !Utils.isDeepEqual(nextProps.formState, this.props.formState)
    if (this.props.onChange && didUpdate) {
      this.props.onChange(newState(nextProps.formState), this.getFormApi())
    }
}

As the entire formState is compared, onChange would be also called when solely internal meta data is changing.

In my use case, react-form renders forms of data sets whose updates should be persisted on a remote server.

This implementation results in unnecessary PUT requests without actual changes to the data.
In my view, the majority of users would expect onChange callbacks to fire when values are changing.

Suggestions (which obviously highly depend on the desired API):

  • Use solely formState.value for change detection and/or
  • Allow to provide additional callbacks which distinguish better between changed attributes, f.e. onValueChange, onTouchChange, onErrorChange etc.
  • Allow to provide an optional custom change detector which receives the previous/next state.
  • ...
@cmtt cmtt changed the title onChange called when fields are being focused/blured onChange called when fields are being focused/blurred Mar 7, 2018
@joepuzzo
Copy link
Contributor

joepuzzo commented Mar 7, 2018

Hmm maybe we can add another function.. The onChange for the form is designed to work the way it does.. if you blur a field that is a change on the form. You can tie into the native on change at the input level and it will only fire when value changes but i can see how that also would not fit your need. For now you can just check yourself if values changed but i can look into adding this next commit.

@cmtt
Copy link
Author

cmtt commented Mar 8, 2018

Thank you for the effort. 👍

@joepuzzo
Copy link
Contributor

Been really busy lately but this is still on radar.

@jacargentina
Copy link

Also having this issue; this is a REAL problem when for example, by simply playing with moving through the fields without any change going on (same values) and then in my case the asyncValidate i've attached on the Form is firing on each move!

In my case i was expecting that the validation where called when there is real changes to data to be validated, and not always as it is now happening.

@joepuzzo I can make the PR, (have some time) if you bless the solution to this thing; may be an idea @cmtt posted there?

@joepuzzo
Copy link
Contributor

Stand by for the release of my new lib.. informed. Lots of simplifications.. better docs, high test coverage and support for new context api and i got rid of redux dependency. I can add an onValueChange to that

@joepuzzo
Copy link
Contributor

Oh and iterface is very similar. Should be very easy to hop over

@joepuzzo
Copy link
Contributor

joepuzzo commented May 29, 2018

Sneak peak here https://joepuzzo.github.io/informed Just created an issue here https://github.com/joepuzzo/informed/issues/2

@nicoburns
Copy link

@joepuzzo What is the relation of Informed to React Form? Why create a new library rather than a new version of this one?

@joepuzzo
Copy link
Contributor

joepuzzo commented Jun 1, 2018

Wanted to create a clean rewrite and im getting rid of some of the more complex features that 90% of people dont need. Im not the only owner of this lib also, so after talking to others we decided it would be best to create a new one.

@cmtt
Copy link
Author

cmtt commented Jun 5, 2018

@joepuzzo: This is a bit discouraging, as it requires rewriting existing solutions.

A new framework doesn't resolve this specific issue and also react-form underwent already too many API changes in my view.

@jacargentina
Copy link

@joepuzzo: While waiting for the new lib, can we add the same thing you've implemented on informed here, and release?

@joepuzzo
Copy link
Contributor

Informed is now live http://joepuzzo.github.io/informed

@cmtt
Copy link
Author

cmtt commented Jun 18, 2018

Looks like a WONTFIX. Closing issue.

@cmtt cmtt closed this as completed Jun 18, 2018
@joepuzzo
Copy link
Contributor

I added this in informed

@cmtt
Copy link
Author

cmtt commented Jun 18, 2018

@joepuzzo: This does not resolve this issue, as informed is a different project.

@joepuzzo
Copy link
Contributor

Yup. I agree. But im no longer supporting this lib and if my understanding is correct niether is tanner. Therefore the best solution to this issue is to switch. I originally had a seperate form lib that was similar enough to react form that tanner and i decided to merge. Then react form became to large and hard to mantain for the both of us. We agreed that it would be best if i rewrote a more streamlined version as a seperate project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants