feat(input): interpolates form input name attrs for ngModel #4791

caitp · 2013-11-05T20:58:25Z

@frankhuster / @niargh has brought up an issue with using the $index of a repeated element for the name attribute of a form input.

I don't know how common of a use-case it is, but it seems not especially harmful to do a one-time interpolation in post-link.

It shouldn't be a breaking change unless people are using interpolation markers in their names.

caitp · 2013-11-05T20:59:59Z

It would be cool to be able to limit parsing/interpolation to only accept types like string/number for this, and have everything else be the empty string --- But I guess that's complicated, so this is the simplest way to do it.

I don't know how valuable this is to other developers, but it seems to be useful to at least one person.

jamie-pate · 2013-11-25T21:43:25Z

+1

caitp · 2013-11-30T17:57:32Z

I had thought ng-attr-name might be enough for this, but testing with http://plnkr.co/edit/HQHNHCjVqEKmAC1lHvED?p=preview seems to show that it's not (priority issue, I think). (http://plnkr.co/edit/BC7GegiP5MeO65Lniw1X?p=preview works just fine with the patch)

So, updating the ng-attr-* "priority" might be one way to go, but I expect it would be much more problematic than this simple fix.

IgorMinar · 2013-12-07T10:57:57Z

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

IgorMinar · 2013-12-10T21:54:15Z

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

IgorMinar · 2013-12-10T23:12:55Z

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

caitp · 2013-12-15T08:55:02Z

In the mean time, or in case this is never merged, I wrote a plnkr the other showing how you could get this functionality with separate directives, or by decorating the existing ngModel and form directives http://plnkr.co/edit/VnHHEe?p=preview (this version decorates ngModelDirective and formDirective to provide this functionality)

Interpolates the form controll attribute name, so that dynamic form controls (such as those rendered in an ngRepeat) will always have their expected interpolated name. Closes angular#4791

caitp · 2014-09-23T14:33:25Z

@tbosch / @matsko I've un-bitrotten the original patch here, I will add a followup commit which supports renaming form controls in the form itself, although honestly I don't feel like there is a real use case for that

matsko · 2014-09-23T14:34:41Z

Your code breaks with the ngRepeat + $index usecase.

caitp · 2014-09-23T14:47:43Z

how does it "break", you haven't explained this. There are demonstrations of it being used exactly with ngRepeat + $index --- yes, the $index doesn't change, but this is fine

matsko · 2014-09-23T14:49:19Z

So what happens then if the name value isn't present at the time of interpolation? You may end up getting an undefined value.

caitp · 2014-09-23T14:51:39Z

You end up getting the empty string if it's undefined at the time of interpolation. Anyways, I've added a patch which renames the form control, but honestly nobody has ever asked for this, so I don't think we actually need it until someone does. The "support all the use cases and never ship" thing just doesn't seem sensible here

caitp · 2014-09-23T14:53:37Z

Anyways, this does have some safety checks in that it requires form[oldName] to equal the control which is being renamed, which can potentially break things if there are multiple form controls whose name is the empty string.

Lets worry about that when people say it's breaking their apps, really

caitp · 2014-09-23T15:29:52Z

Okay, added another one which supports renaming forms

caitp · 2014-09-23T15:31:05Z

I still kind of feel like this is more feature than people have really asked for and it's probably better to take it one step at a time, but it's in 3 separate commits so we can think about it all separately. @tbosch can you review as well?

shahata · 2014-09-23T17:12:31Z

src/ng/directive/form.js

+                } else {
+                  controller.$name = alias;
+                }
+              });
            }
            if (parentFormCtrl) {
              formElement.on('$destroy', function() {


should invoke unobserveName() here

There's no need to since all watchers are removed when the scope is destroyed. Internally attr.$observe uses $scope.$watch.

Not exactly --- the observers collection stays alive afaik. The attribute interpolation watches are removed automatically though, this is true

it's probably better to unobserve manually rather than accidentally risk keeping a reference alive

whatever makes you guys happy :>

Please don't add unobserve here, as we don't have it at similar places in the code neither and it confuses the reader (ask myself: "why here and not at the other places"...)

Interpolates the form controll attribute name, so that dynamic form controls (such as those rendered in an ngRepeat) will always have their expected interpolated name. Closes angular#4791

caitp · 2014-09-23T18:07:50Z

I think the only thing missing is tests (and possibly functionality) for select controls

tbosch · 2014-09-23T18:37:42Z

src/ng/directive/form.js

@@ -127,6 +128,23 @@ function FormController(element, attrs, $scope, $animate) {
    }
  };

+  // Private API: rename a form control
+  form.$$renameControl = function(control, newName) {


This can be simplified to:

var oldName = control.$name; if (form[oldName] === control) { delete form[oldName]; } form[newName] = control; control.$name = newName;

tbosch · 2014-09-23T18:53:12Z

LGTM when my comments are resolved.
Ah, and please squash into a single commit as the commits are strongly related.

Interpolates the form controll attribute name, so that dynamic form controls (such as those rendered in an ngRepeat) will always have their expected interpolated name. Closes angular#4791 feat(input): rename form controls when name is updated feat(form): support dynamic form validation test(select): add tests for form control interpolation in selectDirective

caitp · 2014-09-23T19:15:08Z

some comments are addressed --- code is simplified a bit.

If it looks good to land I will fix up the commit message and get it into this release.

tbosch · 2014-09-23T19:52:08Z

test/ng/directive/inputSpec.js

+  });
+
+
+  it('should interpolate input names (number)', function() {


This test is not needed, as the change is not dependent on the input type...

Interpolates the form and form control attribute name, so that dynamic form controls (such as those rendered in an ngRepeat) will always have their expected interpolated name. The control will be present in its parent form controller with the interpolated property name, and this name can change when the interpolated value changes. Closes angular#4791

caitp mentioned this pull request Nov 25, 2013

Testing dynamically named inputs inside ng-repeat using $setViewValue() #5125

Closed

petebacondarwin mentioned this pull request Dec 2, 2013

feat(form): interpolate form names #5231

Closed

caitp mentioned this pull request Dec 18, 2013

dynamic element validation #1404

Closed

caitp mentioned this pull request Jan 11, 2014

Add interpolation to form id/name #5736

Closed

2 tasks

lookfirst mentioned this pull request Feb 15, 2014

unique form element names in directive template code #6268

Closed

sjbarker mentioned this pull request Mar 6, 2014

feat(directive): ngName #6569

Closed

btford removed gh: PR labels Aug 20, 2014

caitp force-pushed the master branch from 591bac5 to d713ad1 Compare August 22, 2014 13:50

tbosch force-pushed the master branch from 6c27b89 to 271572c Compare August 30, 2014 00:57

tbosch force-pushed the master branch from a3d7934 to 1418383 Compare September 10, 2014 18:14

petebacondarwin force-pushed the master branch from 02dc2aa to fd2d6c0 Compare September 16, 2014 11:25

caitp force-pushed the issue-4791 branch from 8c606cc to ba0b169 Compare September 23, 2014 14:31

caitp force-pushed the issue-4791 branch from 600664b to 28662d6 Compare September 23, 2014 14:52

caitp force-pushed the issue-4791 branch from 28662d6 to 64abf70 Compare September 23, 2014 15:29

caitp force-pushed the issue-4791 branch from 64abf70 to 84c0343 Compare September 23, 2014 15:30

shahata reviewed Sep 23, 2014
View reviewed changes

caitp force-pushed the issue-4791 branch 5 times, most recently from 58dd9ae to 1b59793 Compare September 23, 2014 18:05

tbosch reviewed Sep 23, 2014
View reviewed changes

caitp force-pushed the issue-4791 branch from 90855b9 to 7ef1d0f Compare September 23, 2014 19:14

tbosch reviewed Sep 23, 2014
View reviewed changes

caitp force-pushed the issue-4791 branch from 7ef1d0f to a8fba75 Compare September 23, 2014 20:01

caitp force-pushed the issue-4791 branch from a8fba75 to 6c89a32 Compare September 23, 2014 20:03

caitp closed this in 729c238 Sep 23, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(input): interpolates form input name attrs for ngModel #4791

feat(input): interpolates form input name attrs for ngModel #4791

caitp commented Nov 5, 2013

caitp commented Nov 5, 2013

jamie-pate commented Nov 25, 2013

caitp commented Nov 30, 2013

IgorMinar commented Dec 7, 2013

IgorMinar commented Dec 10, 2013

IgorMinar commented Dec 10, 2013

caitp commented Dec 15, 2013

caitp commented Sep 23, 2014

matsko commented Sep 23, 2014

caitp commented Sep 23, 2014

matsko commented Sep 23, 2014

caitp commented Sep 23, 2014

caitp commented Sep 23, 2014

caitp commented Sep 23, 2014

caitp commented Sep 23, 2014

shahata Sep 23, 2014

matsko Sep 23, 2014

caitp Sep 23, 2014

caitp Sep 23, 2014

caitp Sep 23, 2014

tbosch Sep 23, 2014

caitp commented Sep 23, 2014

tbosch Sep 23, 2014

tbosch commented Sep 23, 2014

caitp commented Sep 23, 2014

tbosch Sep 23, 2014

		});


		it('should interpolate input names (number)', function() {

feat(input): interpolates form input name attrs for ngModel #4791

feat(input): interpolates form input name attrs for ngModel #4791

Conversation

caitp commented Nov 5, 2013

caitp commented Nov 5, 2013

jamie-pate commented Nov 25, 2013

caitp commented Nov 30, 2013

IgorMinar commented Dec 7, 2013

IgorMinar commented Dec 10, 2013

IgorMinar commented Dec 10, 2013

caitp commented Dec 15, 2013

caitp commented Sep 23, 2014

matsko commented Sep 23, 2014

caitp commented Sep 23, 2014

matsko commented Sep 23, 2014

caitp commented Sep 23, 2014

caitp commented Sep 23, 2014

caitp commented Sep 23, 2014

caitp commented Sep 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Sep 23, 2014

Choose a reason for hiding this comment

tbosch commented Sep 23, 2014

caitp commented Sep 23, 2014

Choose a reason for hiding this comment