Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix order of changing state #108

Merged
merged 7 commits into from
Jun 26, 2019
Merged

fix order of changing state #108

merged 7 commits into from
Jun 26, 2019

Conversation

yuki-mt
Copy link
Member

@yuki-mt yuki-mt commented May 24, 2019

What is this PR for?

fix error caused by second requests of API,
Currently display the previous result as notification because
prevState of getDerivedStateFromProps is actually not previous state, so followings happen.

(e.g. upload model)

  1. when call setState({submitting: true}) in onSubmit, getDerivedStateFromProps is called
  2. from the second request, uploadModelStatus is already success or fail, depending on the previous result
  3. nextState.submitting is true, so display the previous result as notfiication, not waiting for the current API request to return response

This PR includes

  • remove state.form because it is not used ( ca718a2 )
  • change from prevState to nextState` in getDerivedStateFromProps ( e0e1f75 )
  • modify order of changing state ( 3393018 and 34a6a57)
  • quit using isSubmitting because it does not change when API request failed ( 89e3ddc )

What type of PR is it?

Bugfix

What is the issue?

N/A

How should this be tested?

write a method and a sample of command

@yuki-mt yuki-mt requested a review from keigohtr May 24, 2019 10:29
@yuki-mt yuki-mt self-assigned this May 24, 2019
@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #108 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #108   +/-   ##
=======================================
  Coverage   83.73%   83.73%           
=======================================
  Files          43       43           
  Lines        2281     2281           
=======================================
  Hits         1910     1910           
  Misses        371      371

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 311f514...663319b. Read the comment docs.

@yuki-mt yuki-mt changed the title fix model modal state fix order of changing state May 24, 2019
Copy link
Member

@keigohtr keigohtr left a comment

Choose a reason for hiding this comment

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

Removing unnecessary code (e.g. state.form and return of onSubmit()) is a great work.
I made a comment, could you check it?

@@ -88,7 +88,7 @@ class AddModelFileFormImpl extends React.Component<AddModelFileFormProps, AddMod
validationSchema={AddModelFileSchema}
onSubmit={this.onSubmit}
onReset={this.onCancel}>
{({ errors, touched, setFieldValue, isSubmitting }) => (
{({ errors, touched, setFieldValue }) => (
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to rewrite here. You can write this like this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

@keigohtr
I changed here. 663319b

Please review it again.

Copy link
Member

@keigohtr keigohtr left a comment

Choose a reason for hiding this comment

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

LGTM

Current implementation of isSubmitting, state.submitting and state.submitted is complicated. We need to fix them in the future...

@yuki-mt yuki-mt merged commit 4508c69 into master Jun 26, 2019
@yuki-mt yuki-mt deleted the fix/model-modal-state branch June 26, 2019 08:36
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants