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

Add onReset callback. #328

Merged
merged 3 commits into from
Feb 1, 2018
Merged

Add onReset callback. #328

merged 3 commits into from
Feb 1, 2018

Conversation

skvale
Copy link
Collaborator

@skvale skvale commented Jan 6, 2018

Since onSubmit had a callback, onReset seemed like it should have an equivalent one.

Fixes: #325

@skvale skvale changed the title feat(formik): Add onReset callback. Add onReset callback. Jan 6, 2018
@jaredpalmer
Copy link
Owner

Can we reopen this against next branch?

@skvale skvale changed the base branch from master to next January 18, 2018 00:18
@skvale skvale changed the base branch from next to master January 18, 2018 00:18
@skvale skvale changed the base branch from master to next January 18, 2018 00:26
@skvale
Copy link
Collaborator Author

skvale commented Jan 18, 2018

I just force pushed and changed the base

@stephenrichard
Copy link

Hey @skvale ,
I'm working on multiple forms and need to execute some code when calling onReset. Do you have some time to look at the conflict and merge this PR to Formik ?
Thank you for the feature by the way ! 😊

@skvale
Copy link
Collaborator Author

skvale commented Jan 29, 2018

I fixed the conflicts, but I am still waiting for a code review.

@jaredpalmer
Copy link
Owner

Reviewing now

@jaredpalmer jaredpalmer self-requested a review January 29, 2018 18:33
Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fantastic.

@@ -423,6 +429,22 @@ export class Formik<
);
};

getFormikActions = (): FormikActions<Values> => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. this was long overdue.

src/formik.tsx Outdated
@@ -550,6 +560,9 @@ export class Formik<
};

handleReset = () => {
if (this.props.onReset) {
this.props.onReset(this.state.values, this.getFormikActions());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow this to async?

Some ideas:

  • sniff for a promise
  • expose a done() argument
  • force this to be sync

i think sniffing for a promise is the move.

naive implementation:

if (this.props.onReset) {
  const maybePromise = this.props.onReset(this.state.values, this.getFormikActions());
  if (isPromise(maybePromise)) {
    // do we pass values returned by the promise?? hmmm. 
    maybePromise.then(this.resetForm) 
  } else {
    this.resetForm();
 }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (this.props.onReset) {
      const maybePromisedOnReset = (this.props.onReset as any)(
        this.state.values,
        this.getFormikActions()
      );
 
      if (isPromise(maybePromisedOnReset)) {
        (maybePromisedOnReset as Promise<any>).then(this.resetForm);
      } else {
        this.resetForm();
      }
    }

Maybe onReset returning new nextValues for resetForm will be useful ?

expect(onReset).toHaveBeenCalledWith(
{ foo: 'bar', bar: 'foo' },
expect.objectContaining({
resetForm: expect.any(Function),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

This passes returned promise data to resetForm, allowing the promise to add new nextValues.
@jaredpalmer jaredpalmer merged commit 5017053 into jaredpalmer:next Feb 1, 2018
jaredpalmer added a commit that referenced this pull request Feb 1, 2018
* Close #200. Add Field level validation

* Regenerate doctoc

* Remove handleChangeValue completely

* v0.11.0-alpha.1

* Add missing name in call to setFieldError (#290)

* v0.11.0-alpha.2

* #281 Add array helpers MVP, update ts setup for better DX (#298)

* v0.11.0-alpha.3

* Actually export FieldArray

* v0.11.0-alpha.4

* Upgrade to React 16, TypeScript 2.6.2 (#300)

* #283 Update to React 16, TypeScript 2.6.2

* Update Travis build to use npm instead of yarn

* Move back to yarn (#301)

* Fix travis script to use yarn.

* Add form to TS types of FieldArray

* v0.11.0-beta.1

* Close #281 #155. Add docs about array helpers

* Add field array examples and update toc

* Fix unshift in FieldArray (#337)

* Fix FieldArray component prop (#341)

* Fix FieldArray component prop

* nit: test names [skip ci]

* Allow bracket path support (#334)

* feat(Field): Allow bracket path support

* chore(deps): Add types for lodash.topath

* chore(deps): yarn.lock

* Fix #280. Fix the definition dirty and isValid (#365)

* Fix #280. Change the meaning of dirty and isValid

* Lock in to path dep

* Update tests for new definition of dirty

* Update docs

* Fixed typo in FieldArray code (#380)

* Fix setDeep mutations with nested values (#373)

* Fix start task

* Get rid of mutators file

* v0.11.0-rc.1

* #285 #122 Add example multistep / form wizard

* #378 remove setFieldTouched from FieldArray

* v0.11.0-rc.2

* #223 Fix checkbox validation error (again)

* Add onReset callback. (#328)

* feat(formik): Add onReset callback.

* feat(formik): Use promise sniffing for onReset prop.

This passes returned promise data to resetForm, allowing the promise to add new nextValues.
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

Successfully merging this pull request may close these issues.

3 participants