Feat: ngRef directive to publish controllers, or elements into current scope #16511

Narretz · 2018-03-26T15:57:17Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feat

What is the new behavior (if this is a feature change)?
publish a component's or element-directive's, or jqlite wrapped DOM element into the containing scope

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

This is a rebased and updated version of #14080 with the following changes:

publish the jqlite-wrapped DOM element instead of the raw DOM element. In AngularJS, we don't deal with raw DOM elements, so this would create an inconsistency
introduce a ngRefElement attribute that indicates that you always want to publish the jqlite-wrapped DOM element, even if a controller exists

googlebot · 2018-03-26T15:57:22Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-03-26T15:57:23Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

jbedard · 2018-03-26T20:18:20Z

src/ng/directive/ngRef.js

+          // only remove it if value has not changed,
+          // carefully because animations (and other procedures) may duplicate elements
+          if (getter(scope) === refValue) {
+            setter(scope,  null);


extra space

jbedard · 2018-03-26T20:18:34Z

src/ng/directive/ngRef.js

+      var setter = getter.assign;
+
+      return function(scope, element, attrs) {
+        var refValue = null;


I don't think the null value is necessary

jbedard · 2018-03-26T20:18:49Z

src/ng/directive/ngRef.js

+
+      // get the expression for value binding
+      var getter = $parse(tAttrs.ngRef);
+      var setter = getter.assign;


Should we throw a nonassign if the expression can not be assigned to?

gkalpak · 2018-03-27T09:10:05Z

src/ng/directive/ngRef.js

+ * to the given property in the current scope. If no controller exists, the jqlite-wrapped DOM
+ * element will be added to the scope.
+ *
+ * If the element with ngRef is destroyed `null` is assigned to the property.


ngRef --> `ngRef`

gkalpak · 2018-03-27T09:15:24Z

src/ng/directive/ngRef.js

+ *     </div>
+ *   </file>
+ *   <file name="index.js">
+ *     angular.module('myApp', []);


While this might be better for a real app, having multiple, tiny files in examples is harder to follow. I would put everything in one JS file (and avoid unnecessary redirection, e.g.:

angular. module('myApp', []). component('myToggle', { controller: function MyToggleController(...) { ... } });

gkalpak · 2018-03-27T09:16:05Z

src/ng/directive/ngRef.js

+ *        var toggle = element(by.buttonText('Toggle'));
+ *        expect(toggle.evaluate('myToggle.isOpen()')).toEqual(false);
+ *        toggle.click();
+ *        expect(toggle.evaluate('myToggle.isOpen()')).toEqual(true);


Wow, I didn't know about the evaluate method. Nice 😃

gkalpak · 2018-03-27T09:16:20Z

src/ng/directive/ngRef.js

+ *
+ * @example
+ * ### ngRef inside scopes
+ * This example shows how new scopes limits


I don't understand this sentence at all :-)

gkalpak · 2018-03-27T09:17:37Z

src/ng/directive/ngRef.js

+ *        <div>outerToggle.isOpen(): {{outerToggle.isOpen() | json}}</div>
+ *     </li>
+ *     </ul>
+ *     <div>ngRepeat.isOpen(): {{ngRepeatToggle.isOpen() | json}}</div>


ngRepeat --> ngRepeatToggle

gkalpak · 2018-03-27T09:18:02Z

src/ng/directive/ngRef.js

+ *        <div>ngIfToggle.isOpen(): {{ngIfToggle.isOpen() | json}}</div>
+ *        <div>outerToggle.isOpen(): {{outerToggle.isOpen() | json}}</div>
+ *     </div>
+ *     <div>ngIf.isOpen(): {{ngIf.isOpen() | json}}</div>


ngIf --> ngIfToggle

gkalpak · 2018-03-27T09:18:43Z

src/ng/directive/ngRef.js

+ * </example>
+ *
+ */
+var ngRefDirective = ['$parse',function($parse) {


Space after ,.

gkalpak · 2018-03-27T09:23:01Z

src/ng/directive/ngRef.js

+ *        });
+ *      });
+ *   </file>
+ * </example>


From the examples, it looks like we suggest putting these stuff on the scope (I hope we don't).
It would be better if showcased how to assign the values into controllers, instead.

Sure. I didn't write the examples, but can fix them.

I think that in the simple case where you just want to use the value inside the HTML only it makes sense to assign the ref to the scope.

I've changed the 2nd example to use $ctrl, but not the first. I think we can keep it like this - the whole docs are riddled with "old style" examples

petebacondarwin · 2018-03-28T12:21:23Z

src/ng/directive/ngRef.js

+ * @param {string} ngRef property name - A valid AngularJS expression identifier to which the
+ *                       controller or jqlite-wrapped DOM element will be bound.
+ * @param {string=} ngRefElement void - If specified, ngRef will always bind the jqlite-wrapped
+ *                               DOM-element even if a controller is available.


What does the void - bit mean in the description here?

It is supposed to signify that the attribute takes no value. :)

dcherman · 2018-04-05T03:22:38Z

src/ng/directive/ngRef.js

+      return function(scope, element, attrs) {
+        var refValue = null;
+
+        if ('ngRefElement' in attrs) {


~~Out of curiosity, would this preclude the use of both ngRef and ngRefElement used in conjunction to get both the controller and element?~~

Never mind, it's late. Thought this was a separate directive, then realized it was an option.

Narretz · 2018-04-06T10:55:00Z

The API design has changed after discussing it with @petebacondarwin - there is now a ngRefRead attribute that can be used to assign a specific directive's controller or the element.

The default behavior is still that the component /element directive's controller, or if there's no component / directive, the element is assigned.

petebacondarwin · 2018-05-22T10:16:35Z

@Narretz is this now ready for a full review?

Narretz · 2018-05-22T14:18:46Z

@petebacondarwin not quite. I'll try to finish it tonight.

petebacondarwin

Few minor comments

petebacondarwin · 2018-05-25T09:54:46Z

docs/content/error/ngRef/nonassign.ngdoc

+
+```
+
+To resolve this error, use a path expression that are assignable:


that is assignable

petebacondarwin · 2018-05-25T10:03:56Z

test/ng/directive/ngRefSpec.js

+    expect($rootScope.mySpan[0].textContent).toBe('my text');
+  }));
+
+  it('should nullify the dom element value if it is destroyed', inject(function($compile, $rootScope) {


Is this test description accurate? It implies, to me at least, that the value of the DOM element is being set to null.
I think we are actually setting the value of the expression to null, right?

petebacondarwin · 2018-05-25T10:05:45Z

test/ng/directive/ngRefSpec.js

+
+      expect($rootScope.myDirective).toBe(myDirectiveController);
+    });
+  });


Is it worth having a test that shows the element directive / component is chosen over any additional attribute directives if no ngRefRead attribute is available?

petebacondarwin · 2018-05-25T10:07:05Z

test/ng/directive/ngRefSpec.js

+      });
+
+      inject(function($compile, $rootScope) {
+        var template = '<my-component ng-ref="myComponent">{{myComponent.text}}</my-component>';


Woah! This makes you think :-)

petebacondarwin · 2018-05-25T10:11:40Z

src/ng/directive/ngRef.js

+ * @restrict A
+ *
+ * @description
+ * The `ngRef` attribute tells AngularJS to assign the controller of a component (or an element-directive)


We can also assign attribute directives if we use ngRefRead so this sentence is not entirely accurate.

petebacondarwin · 2018-05-25T10:13:05Z

src/ng/directive/ngRef.js

+ *        });
+ *      });
+ *   </file>
+ * </example>


I think that in the simple case where you just want to use the value inside the HTML only it makes sense to assign the ref to the scope.

Narretz · 2018-05-26T14:11:57Z

I've updated the PR with your suggestions @petebacondarwin

gkalpak · 2018-05-31T12:49:00Z

test/helpers/matchers.js

@@ -313,6 +313,7 @@ beforeEach(function() {

      function generateCompare(isNot) {
        return function(actual, namespace, code, content) {
+


gkalpak · 2018-05-31T21:08:43Z

src/ng/directive/ngRef.js

+
+var ngRefDirective = ['$parse', function($parse) {
+  return {
+    priority: -1,


Why the -1?

This is needed for transclusion compatibility

Is it because you are doing work in the compile function?
(Does this work have to be done there in the first place?)

It doesn't look like it's because of the compile fn. Without the priority of -1, the link function receives the transclusion comment instead of the actual element if the element has "element transclusion" on it. I haven't looked into why it happens like this, though.

gkalpak · 2018-05-31T21:12:40Z

src/ng/directive/ngRef.js

+        var refValue;
+        var controller;
+
+        if ('ngRefRead' in attrs) {


Use .hasOwnProperty() instead (to avoid traversing the prototype chain)?

gkalpak · 2018-05-31T21:18:26Z

src/ng/directive/ngRef.js

+          if (attrs.ngRefRead === '$element') {
+            refValue = element;
+          } else {
+            controller = element.data('$' + attrs.ngRefRead + 'Controller');


Why have two different variables (refValue, controller) and not assign to refValue directly?

gkalpak · 2018-05-31T21:20:04Z

src/ng/directive/ngRef.js

+        // when the element is removed, remove it (nullify it)
+        element.on('$destroy', function() {
+          // only remove it if value has not changed,
+          // carefully because animations (and other procedures) may duplicate elements


How do you remove it carefully? 😁

@drpicox

…o scope Thanks to @drpicox for the original implementation: PR angular#14080

googlebot · 2018-06-04T08:09:54Z

CLAs look good, thanks!

googlebot · 2018-06-04T08:09:54Z

CLAs look good, thanks!

petebacondarwin · 2018-06-04T12:33:59Z

The Travis failures look pretty legitimate to me.

…ments to scope

@drpicox

Thanks to @drpicox for the original implementation: PR #14080 Closes #16511

alirezamirian · 2018-06-11T11:20:00Z

👍
Once, I had written an article about such feature :)
https://medium.com/@alireza.mirian/mimicking-angular-template-references-in-angularjs-with-a-10-line-directive-c0687a3ddaa6

Narretz added type: feature component: misc core labels Mar 26, 2018

Narretz added this to the 1.6.x milestone Mar 26, 2018

googlebot added the cla: no label Mar 26, 2018

Narretz changed the title ~~Feat ng ref~~ Feat: ngRef directive Mar 26, 2018

Narretz changed the title ~~Feat: ngRef directive~~ Feat: ngRef directive to publish component controllers into current scope Mar 26, 2018

Narretz changed the title ~~Feat: ngRef directive to publish component controllers into current scope~~ Feat: ngRef directive to publish controllers, or elements into current scope Mar 26, 2018

jbedard reviewed Mar 26, 2018

View reviewed changes

gkalpak reviewed Mar 27, 2018

View reviewed changes

petebacondarwin reviewed Mar 28, 2018

View reviewed changes

dcherman reviewed Apr 5, 2018

View reviewed changes

Narretz modified the milestones: 1.6.x, 1.7.x Apr 12, 2018

petebacondarwin self-assigned this May 14, 2018

Narretz force-pushed the feat-ngRef branch 3 times, most recently from 1e610f1 to 935bb71 Compare May 24, 2018 09:52

Narretz added the needs: review label May 24, 2018

petebacondarwin approved these changes May 25, 2018

View reviewed changes

gkalpak reviewed May 31, 2018

View reviewed changes

Narretz mentioned this pull request Jun 1, 2018

feat(ngAsDirective): new as directive to publish component controllers into current scope #14080

Closed

3 tasks

Narretz force-pushed the feat-ngRef branch from 761aaed to d693d5c Compare June 4, 2018 08:08

feat(ngRef): directive to publish directive controllers or elements t…

00bcf19

…o scope Thanks to @drpicox for the original implementation: PR angular#14080

Narretz force-pushed the feat-ngRef branch from d693d5c to 00bcf19 Compare June 4, 2018 08:09

googlebot added cla: yes and removed cla: no labels Jun 4, 2018

Narretz added 2 commits June 4, 2018 18:44

fixup! feat(ngRef): directive to publish directive controllers or ele…

af202ad

…ments to scope

fixup! feat(ngRef): directive to publish directive controllers or ele…

e5c0504

…ments to scope

Narretz merged commit aa6adc7 into angular:master Jun 5, 2018

Narretz added a commit that referenced this pull request Jun 6, 2018

feat(ngRef): add directive to publish controller, or element into scope

bf841d3

Thanks to @drpicox for the original implementation: PR #14080 Closes #16511

dependencies bot mentioned this pull request Jun 8, 2018

Update 6 dependencies from npm JetBrains/ring-ui#281

Closed


		```

		To resolve this error, use a path expression that are assignable:

		@@ -313,6 +313,7 @@ beforeEach(function() {

		function generateCompare(isNot) {
		return function(actual, namespace, code, content) {

Feat: ngRef directive to publish controllers, or elements into current scope #16511

Feat: ngRef directive to publish controllers, or elements into current scope #16511

Conversation

Narretz commented Mar 26, 2018

googlebot commented Mar 26, 2018

googlebot commented Mar 26, 2018

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

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

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

dcherman Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Narretz commented Apr 6, 2018 • edited Loading

petebacondarwin commented May 22, 2018

Narretz commented May 22, 2018

petebacondarwin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented May 26, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 4, 2018

googlebot commented Jun 4, 2018

petebacondarwin commented Jun 4, 2018

alirezamirian commented Jun 11, 2018

dcherman Apr 5, 2018 •

edited

Loading

Narretz commented Apr 6, 2018 •

edited

Loading