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

Feature proposal: clear action creator #3055

Closed
timhwang21 opened this issue Jun 13, 2017 · 9 comments
Closed

Feature proposal: clear action creator #3055

timhwang21 opened this issue Jun 13, 2017 · 9 comments
Labels
enhancement improvements in current API

Comments

@timhwang21
Copy link
Contributor

timhwang21 commented Jun 13, 2017

Are you submitting a bug report or a feature request?

Feature request

A common pattern I'm running into for dependent fields (e.g. field B's value depends on field A having a value) is that I want to clear field B when field A changes.

So I do something like:

// Assume field B, C, and D are conditional and A having a value
handleFieldAChange(event, value) {
  const { change } = this.props;

  if (!value) { // if field a was emptied
    change('b', null);
    change('c', null);
    change('d', null);
    untouch('b', 'c', 'd');
  }
}

change doesn't support passing in multiple fields currently. In my case, I know that the value for each changed field will be null, so an action creator clear() (or some better name) with the same signature as untouch() would be very useful:

handleFieldAChange(event, value) {
  const { clear } = this.props;

  if (!value) {
    clear('b', 'c', 'd');
  }
// Behavior: args.forEach(arg => { blur(arg); change(arg, null); })

Alternative / additional suggestion: Change the argument order of change to (form:string, value:any, ...fields:string), to bulk change values. The downside is that similar action creators will need to be changed to be consistent with the signature.

Is there a better existing way that I'm missing? If not, would there be interest in this functionality? If so I can take a crack at it over the weekend. Thanks!

@danielrob
Copy link
Collaborator

Seems sensible to me, I'd say go for it!

@danielrob
Copy link
Collaborator

danielrob commented Aug 6, 2017

Note as well the linked issue^.. maybe there are other cases for having bulk setting of formValues.

@timhwang21
Copy link
Contributor Author

Hm, I just saw the issue, so maybe a changeMany or something would be more general-purpose. Thanks! (By the way, I'm not sure what the best way to proceed on this is -- I've filed another issue #3295)

@danielrob
Copy link
Collaborator

setFormValues is another name suggestion ;).

@danielrob
Copy link
Collaborator

Consider also #2224

@timhwang21
Copy link
Contributor Author

@danielrob So I'm running into an issue in the implementation of a bulk change action, and was hoping one of the maintainers could chime in... (cc @erikras)

The issue: the function signature for actions that change multiple fields is ...fields, taking rest arguments instead of an array for convenience/ nicer syntax, I guess. This means that the list of fields must be the last arguments passed.

However, internally the change action also has a functionality for custom touch and persistentSubmitErrors. The signature of this function is (form: string, field: string, value: any, touch: boolean, persistentSubmitErrors: boolean).

Any sort of bulk change action can't have touch and persistentSubmitErrors as the last argument while also accepting spread field names vs. an array. If these two arguments were moved to the front, however, the exported action creator would have two arguments the user shouldn't care about, as createReduxForm should be responsible for passing these arguments based on configuration.

Not supplying these arguments at all would make the bulk change action behave subtly different from the change action. Bug reports galore 😱

The bulk change action could simply dispatch an array of change actions, which replicates how users are currently using change() to set multiple fields to a given value. But IMO the reducer should ideally handle all values at once for better performance (unless the form values object has some immutable-based optimization for deep-setting values? I've seen the set and get functions but haven't dived into their implementation details).

Thanks!

dvdmgl added a commit to dvdmgl/redux-form that referenced this issue Oct 31, 2017
clears fields values by a given array

Issue: redux-form#3055, Issue: redux-form#2224
dvdmgl added a commit to dvdmgl/redux-form that referenced this issue Oct 31, 2017
clears fields values by a given array

Issue: redux-form#3055, Issue: redux-form#2224
erikras pushed a commit that referenced this issue Nov 2, 2017
clears fields values by a given array

Issue: #3055, Issue: #2224
@ste93cry
Copy link

ste93cry commented Nov 2, 2017

@erikras Do you have any ETA on when this feature will be released in a stable version?

@erikras
Copy link
Member

erikras commented Nov 27, 2017

Fix published in v7.2.0.

@lock
Copy link

lock bot commented Dec 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement improvements in current API
Projects
None yet
Development

No branches or pull requests

4 participants