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 support for unsaved changes to EditableList. STCOM-26. #23

Merged
merged 11 commits into from
Jul 27, 2017

Conversation

mkuklis
Copy link
Contributor

@mkuklis mkuklis commented Jul 24, 2017

@JohnC-80 and @zburke this PR introduces a new version of <EditableList> with a support for navigation check in cases when the unsaved changes are detected. @JohnC-80 the work is based on FieldArray (thank you for recommending it). It allowed for easier state management for each item inside the form.

I tried to keep the current API for <EditableList> aligned with the previous version so it stays compatible with the apps. It seems like things are working well but it would be great if you could review it. Thank you!

@mkuklis mkuklis requested review from zburke and JohnC-80 July 24, 2017 08:53
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

This looks great to me. Nice work!

One thing I noticed: if I click the “Edit” button for multiple items, change multiple items, then click one “Save” button, the edits for the other items disappear. I suspect this is not a common use case as only a BOFH would bother deliberately trying break it this way, but it is something to know about, if not to bother fixing. I suspect this is related to using the FieldArray rather than a series of independent forms. I found FieldArray to be double-edged in that regard when I first started looking at this before it landed on your plate.

Everything in the code looks fine to me — even ESLint runs cleanly!

@mkuklis
Copy link
Contributor Author

mkuklis commented Jul 26, 2017

Thank you for the review @zburke and for catching these extra issues! It appears that there is another flag in redux-form called keepDirtyOnReinitialize which keeps the dirty fields unchanged on reinitialization. I found it via this discussion: redux-form/redux-form#370
This should fix these additional issues. @JohnC-80 I'm going to merge this PR to master. I hope you don't mind :)

@JohnC-80
Copy link
Contributor

JohnC-80 commented Jul 27, 2017 via email

@mkuklis
Copy link
Contributor Author

mkuklis commented Jul 27, 2017

@JohnC-80 Thank you for your review and for catching this issue. Unfortunately it looks like this started happening after I added keepDirtyOnReinitialize. It also looks like keepDirtyOnReinitialize doesn't play well with our own navigationCheck. I'm going to remove it for now. It means that what @zburke has discovered will be still reproducible but it shouldn't be a big deal (if it is we may take another take on this :)).

Fixes UIIT-36, UIIT-48, UIU-148 and UIU-79
@mkuklis mkuklis merged commit 6083832 into master Jul 27, 2017
@mkuklis mkuklis deleted the stcom-26-editable-list branch July 27, 2017 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants