Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

fix(validations): Fix handling of async validations as well as order of onChange notification #57

Merged
merged 3 commits into from
Aug 13, 2019
Merged

fix(validations): Fix handling of async validations as well as order of onChange notification #57

merged 3 commits into from
Aug 13, 2019

Conversation

schlegz
Copy link
Contributor

@schlegz schlegz commented Aug 13, 2019

It was discovered when refactoring the AddressForm component that the onChange notification in FormBot was called before field updating/validation was complete. This is an issue if in an onChange handler for FormBot you are expecting to test for form validity (i.e. updating your UI according to the validity of a recent change to form data). Additionally the return from the async validation block was triggering the finally handler causing premature promise resolution and triggering the onChange handler before validations were complete.

@schlegz schlegz added the bug Something isn't working label Aug 13, 2019
@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #57 into master will decrease coverage by 0.3%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   67.19%   66.89%   -0.31%     
==========================================
  Files          22       22              
  Lines         445      447       +2     
  Branches       92       92              
==========================================
  Hits          299      299              
- Misses        116      118       +2     
  Partials       30       30
Impacted Files Coverage Δ
src/Form/Formbot.js 2.89% <0%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28619de...fc8ff5b. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Aug 13, 2019 •
edited
Loading

Codecov Report

Merging #57 into master will decrease coverage by 0.3%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   67.19%   66.89%   -0.31%     
==========================================
  Files          22       22              
  Lines         445      447       +2     
  Branches       92       92              
==========================================
  Hits          299      299              
- Misses        116      118       +2     
  Partials       30       30
Impacted Files Coverage Δ
src/Form/Formbot.js 2.89% <0%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28619de...fc8ff5b. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 13, 2019 •
edited
Loading

Codecov Report

Merging #57 into master will decrease coverage by 0.3%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   67.19%   66.89%   -0.31%     
==========================================
  Files          22       22              
  Lines         445      447       +2     
  Branches       92       92              
==========================================
  Hits          299      299              
- Misses        116      118       +2     
  Partials       30       30
Impacted Files Coverage Δ
src/Form/Formbot.js 2.89% <0%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28619de...13f600f. Read the comment docs.

cehsu
cehsu reviewed Aug 13, 2019
View reviewed changes
Copy link
Contributor

@cehsu cehsu left a comment

Choose a reason for hiding this comment

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

LGTM, I will QA. It seems like this component could use some tests, but that can be handled in a separate PR.

erikshestopal
erikshestopal approved these changes Aug 13, 2019
View reviewed changes
src/Form/Formbot.js Outdated Show resolved Hide resolved
choochootrain
choochootrain approved these changes Aug 13, 2019
View reviewed changes
Copy link

@choochootrain choochootrain left a comment

Choose a reason for hiding this comment

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

lgtm, if its quick would be nice to update the formbot example to showcase async validation with something simple like a setTimeout

@schlegz schlegz requested a review from choochootrain August 13, 2019 18:08
choochootrain
choochootrain approved these changes Aug 13, 2019
View reviewed changes
Copy link

@choochootrain choochootrain left a comment

Choose a reason for hiding this comment

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

gogogogo

@schlegz schlegz merged commit 9183b3a into master Aug 13, 2019
@schlegz schlegz deleted the fix/async-validation-handling branch August 13, 2019 18:10
mathewmorris added a commit that referenced this pull request Aug 23, 2019
@mathewmorris
Merge branch 'v5' into feat/patient-rd-dropdown
8eb24ea 
* v5:
  feat(Button): Icon support and default style updates (#51)
  feat(Colors): Updates colors (#60)
  chore(release): 4.1.6
  fix(FormError): Fix crash in case context is null (#59)
  chore(release): 4.1.5
  fix: Use updater function in setState for Formbot updates (#58)
  chore(release): 4.1.4
  fix(validations): Fix handling of async validations as well as order of onChange notification (#57)
  chore(release): 4.1.3
  Async Formbot Validations (#56)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cehsu cehsu cehsu approved these changes

@erikshestopal erikshestopal erikshestopal approved these changes

@choochootrain choochootrain choochootrain approved these changes

@kylealwyn kylealwyn Awaiting requested review from kylealwyn

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schlegz @choochootrain @erikshestopal @cehsu