Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(formController): do not let removed control effect form #9037

Closed
wants to merge 3 commits into from
Closed

fix(formController): do not let removed control effect form #9037

wants to merge 3 commits into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Sep 12, 2014

This PR fixes some issues with manually removing and adding controls from forms:

  1. Validity changes in removed control do not effect form validity
  2. Validity of existing control is taken into account when adding control to form

Closes #9035

forEach(ctrl[key], function(value, validationErrorKey) {
ctrl.$$getParentForm().$setValidity(validationErrorKey, combinedState, ctrl);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

@shahata: I don't expect it to have a significant impact, but wouldn't it be better to "cache" the parentForm outside the forEach loops:

var formCtrl = ctrl.$$getParentForm();
forEach(stateMap, function(combinedState, key) {
  forEach(ctrl[key], function(value, validationErrorKey) {
    formCtrl.$setValidity(validationErrorKey, combinedState, ctrl);
  });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks

@shahata shahata changed the title Fix remove control fix(formController): do not let removed control to effect form Sep 12, 2014
@shahata shahata changed the title fix(formController): do not let removed control to effect form fix(formController): do not let removed control effect form Sep 15, 2014
@jeffbcross
Copy link
Contributor

@tbosch can you take a look?

@@ -58,6 +58,61 @@ describe('form', function() {
expect(form.alias).toBeUndefined();
});

it('should not be effected by input controls that were removed from the form', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

affected

@btford btford self-assigned this Sep 29, 2014
@shahata
Copy link
Contributor Author

shahata commented Sep 30, 2014

@btford fixed + rebased

@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.4, 1.3.0-rc.5 Oct 2, 2014
@IgorMinar
Copy link
Contributor

We decided to that this was an invalid use of the $addFromControl / $removeFormControl APIs. Those apis are public for the situations when people build their own custom form controls. They are not intended for application code to add/remove existing form controls from the form.

@btford I believe we should close this unless you have some pending task related to this PR.

@btford
Copy link
Contributor

btford commented Oct 8, 2014

@IgorMinar++

Thanks for your work on this though, @shahata. FWIW, 0563a0a landed since it's still a legit improvement.

Closing.

@btford btford closed this Oct 8, 2014
@shahata
Copy link
Contributor Author

shahata commented Oct 9, 2014

Hmmm, I thought that this change might be particularly useful if we decide to support the form attribute (see #8917)

@tbosch
Copy link
Contributor

tbosch commented Oct 9, 2014

Interesting, didn't know this. Let's revisit this PR when we work on #8917....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in angular.FormController when nesting ng-form's
7 participants