This repository has been archived by the owner on Apr 12, 2024. It is now read-only.
feat(form.FormController): add $getControls() #16601
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes angular#14749 Closes angular#14517 Closes angular#13202
Narretz
force-pushed
the
feat-form-controls
branch
from
d198a2d
to
30db7f5
Compare
gkalpak
suggested changes
Jun 15, 2018
src/ng/directive/form.js
Outdated
| @@ -4,6 +4,7 @@ | |||
| */ | |||
| var nullFormCtrl = { | |||
| $addControl: noop, | |||
| $getControls: noop, | |||
There was a problem hiding this comment.
I think this should return an empty array (to respect the "contract" that $getControls() returns an array).
Maybe change it to valueFn([]).
src/ng/directive/form.js
Outdated
| * @description | ||
| * This method returns a **shallow copy** of the controls that are currently part of this form | ||
| * ({@link form.FormController `FormController`} / | ||
| * {@link ngModel.NgModelController `NgModelController`}) . This can be used |
There was a problem hiding this comment.
I assume here you refer to the fact that the controls can be either FormController or NgModelController instances. Having this parenthesis after "of this form" is a little confusing.
I think it makes sense to be more explicit about this in the docs (because not all people might realize controls can actually be other FormControllers) and also mention that if you want nested controls, you have to recursively call $getControls() on sub-forms.
Narretz
force-pushed
the
feat-form-controls
branch
from
b6d0284
to
ab8f14a
Compare
|
@gkalpak Pr has been updated, PTAL |
gkalpak
approved these changes
Jun 18, 2018
test/ng/directive/formSpec.js
Outdated
|
|
||
| scope.$digest(); | ||
|
|
||
| var form = doc, |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #14749
Closes #14517
Closes #13202
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feat
What is the current behavior? (You can also link to an open issue here)
there's no straight forward way to get all controls of a form
What is the new behavior (if this is a feature change)?
you can get a shallow copy of the form controls
Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
Other information:
I decided against a synchronized copy (#14749 (comment)) because I don't see the clear benefit over a simple shallow copy + is slightly more involved to implmenent.