Destroy task #1409

Merged
merged 3 commits into from Feb 10, 2017

Conversation

Projects
None yet
2 participants
@darcatron
Contributor

darcatron commented Jan 27, 2017

This fixes an issue with how the form elements were mapped. If a prior form field isn't used in an updated form, it would throw an undefined error when it tried to access the non-existent field. I tried finding a good way to remove the field without disrupting the rest of the uses of the FormModal component, but I could not find one. Right now it just removes as unused fields from the mapping. If you see a better method, I could def use it

@ssalinas

- default:
- parsed[key] = val;
+ if (element === undefined) {
+ delete this.state.formState[key];

This comment has been minimized.

@ssalinas

ssalinas Jan 27, 2017

Member

we don't want to directly manipulate the state here. Instead we need to eventually pass the new data to setState. We use the return value from this method to set the state correctly on line 153. So, I think if the element is undefined we can just skip that element and move on. It will then not end up in parsed and will be later removed from the state when we do setState

@ssalinas

ssalinas Jan 27, 2017

Member

we don't want to directly manipulate the state here. Instead we need to eventually pass the new data to setState. We use the return value from this method to set the state correctly on line 153. So, I think if the element is undefined we can just skip that element and move on. It will then not end up in parsed and will be later removed from the state when we do setState

let formState = this.parseFormState(this.state.formState);
this.props.onConfirm(formState);
if (!this.props.keepCurrentFormState) {
- const formState = {};
+ formState = {};

This comment has been minimized.

@darcatron

darcatron Jan 27, 2017

Contributor

removed the const because the variable is block scoped and so it would never be saved properly here

@darcatron

darcatron Jan 27, 2017

Contributor

removed the const because the variable is block scoped and so it would never be saved properly here

This comment has been minimized.

@ssalinas

ssalinas Jan 27, 2017

Member

yep, saw that after and deleted my comment, didn't have the diff open wide enough

@ssalinas

ssalinas Jan 27, 2017

Member

yep, saw that after and deleted my comment, didn't have the diff open wide enough

@darcatron

This comment has been minimized.

Show comment
Hide comment
@darcatron

darcatron Jan 27, 2017

Contributor

Updated to ignore unused fields. I tried removing the state entirely and getting it working with just props, but it didn't work easily. It seems possible though. We should re-visit it at another time.

Contributor

darcatron commented Jan 27, 2017

Updated to ignore unused fields. I tried removing the state entirely and getting it working with just props, but it didn't work easily. It seems possible though. We should re-visit it at another time.

@darcatron darcatron added the hs_qa label Feb 8, 2017

@ssalinas ssalinas modified the milestone: 0.14.0 Feb 9, 2017

@darcatron darcatron added the hs_stable label Feb 10, 2017

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Feb 10, 2017

Member

Looking good on stable, LGTM

Member

ssalinas commented Feb 10, 2017

Looking good on stable, LGTM

@ssalinas ssalinas merged commit d335714 into master Feb 10, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the destroy-task branch Feb 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment