feat($compile): explicitly request multi-element directive behaviour #5372

caitp · 2013-12-11T21:00:18Z

also fixes ng-attr cannot add attributes that end in "-start" #6574

With regards to #5370, I think it's a bit crazy to completely disable the use of attributes/directives ending with -start or -end.

I would propose something like this:

angular.module('blah', [])
  .directive('multiElementDir', function() {
    return {
      multiElement: true, // Explicit declaration
      // ...
    };
   });

And, instead of this:

              var directiveNName = ngAttrName.replace(/(Start|End)$/, '');
              if (ngAttrName === directiveNName + 'Start') {
                attrStartName = name;
                attrEndName = name.substr(0, name.length - 5) + 'end';
                name = name.substr(0, name.length - 6);
              }

we could have something like

              var directiveNName = ngAttrName.replace(/(Start|End)$/, '');
              if (hasDirectives.hasOwnProperty(directiveNName) && hasDirectives[directiveNName].multiElement) {
                // Only worry about start/end names if directive is explicitly multiElement
                if (ngAttrName === directiveNName + 'Start') {
                  attrStartName = name;
                  attrEndName = name.substr(0, name.length - 5) + 'end';
                  name = name.substr(0, name.length - 6);
                }
              }

then we could allow both multi-element attributes and attributes ending with -start and -end, which I think would be nice.

jeffbcross · 2013-12-11T19:56:02Z

I like this approach. I agree, there are too many other valid use cases for directives that end with -start or -end.

caitp · 2013-12-11T21:02:29Z

Well, initial patch is up --- there are a few areas where I'm not sure of the best way to go

Which angular core directives should expect this behaviour? All of them? So far I've only changed over the ones which had tests broken because of the change.
Is multiElement a good property name for this, or is there something better?
As far as I can tell, this start/end business shouldn't have affected element directives (like <foo-start>), so I have not written tests for those cases. But if this is not correct, please let me know

rodyhaddad · 2013-12-12T00:30:05Z

The only documented one I see is ngRepeat. Though developers probably used others in production (like ngIf, I know I have).
I think there should be criterias set for this, like we shouldn't include: all event directives, as well everything that modifies its childnodes (ngInclude, ngPluralize, etc.), ...
Technically it should be multiNode, considering that there can be text/comment nodes between the -start/-end elements. But multiElement is more descriptive (hence using it when describing this feature I think)
The -start/-end checks only happen on attributes, so they don't work in element names, classes or comment directive. No need for extra tests :)

Edit: This is a good feature to have. Not all directives can handle multiple elements given to them, so it should be opt-in

caitp · 2013-12-12T01:53:32Z

Some additional docs added for api/ng.$compile --- probably want some in the directive guide as well, and it's probably worded very poorly.

rodyhaddad · 2013-12-19T01:07:46Z

+1

redaemn · 2013-12-19T22:49:16Z

+1 for this PR!!! It really IS crazy to completely disable attributes that ends in -start or -end

gsklee · 2014-01-11T13:49:59Z

+1. The current mechanism that completely invalidates directives ending with these special tokens is a serious oversight. What are we waiting for?

caitp · 2014-01-11T21:34:45Z

since it's a breaking change, it might be hard to get it in before 1.3, and as mentioned it's possible that there are other directives which would benefit from the multiElement behaviour in angular core, which have not been changed in this PR yet.

Also, since it's labeled as a 'feature', it's pushed pretty far back on the list of priorities

By appending directive-start and directive-end to a directive it is now possible to have the directive act on a group of elements. It is now possible to iterate over multiple elements like so: <table> <tr ng-repeat-start="item in list">I get repeated</tr> <tr ng-repeat-end>I also get repeated</tr> </table>

burkeholland · 2014-02-17T17:40:03Z

We need this as well since swallowing the start and end attributes is inadvertently negating part of our API.

kendo-labs/angular-kendo#203

IgorMinar · 2014-03-06T21:59:51Z

src/ng/directive/ngBind.js

@@ -58,7 +58,7 @@ var ngBindDirective = ngDirective(function(scope, element, attr) {
    // jshint -W041
    element.text(value == undefined ? '' : value);
  });
-});
+}, true);


this doesn't look right. ngBind shouldn't need this since it binds to textContent

IgorMinar · 2014-03-06T22:03:14Z

LGTM.

@mhevery ?

caitp · 2014-03-06T22:52:28Z

test/ng/compileSpec.js

-      element = jqLite(element[0].parentNode.childNodes); // reset because repeater is top level.
-      expect(element.text()).toEqual('1A..1B;2A..2B;');
-    }));
+    it('should group on nested groups of same directive', function() {


This spec is grown a bit due to defining a custom directive, so that the same behaviour can be verified without the unexpected multi-element behaviour of the ngBind directive (re: #5372 (comment))

IgorMinar · 2014-07-02T21:44:06Z

ngSwitch/ngSwitchWhen should also be opted-in...

caitp · 2014-07-03T17:05:16Z

@IgorMinar I've determined that ngSwitch can't really opt into this behaviour, because ngSwitchWhen and ngSwitchDefault need to be direct descendants of ngSwitch in order to work correctly (otherwise they can't plug their transclude functions into the ngSwitch controller).

Anyways, I've updated and added some tests for the ng-switch multi-element behaviour which were missing previously, PTAL

(To elaborate on the first paragraph,

<p ng-switch-begin="text"><h1>Story</h1></p>
  <!-- Can't require `ngSwitch` controller, throws, not a direct descendent -->
  <p ng-switch-when="1">{{Page1}}</p>
  <p ng-switch-when="2">{{Page2}}</p> <!-- ditto -->
  <p ng-switch-when="3">{{Page3}}</p> <!-- ditto -->
<p ng-switch-end>FIN</p>

Directives which expect to make use of the multi-element grouping feature introduced in 1.1.6 (angular@e46100f7) must now add the property multiElement to their definition object, with a truthy value. This enables the use of directive attributes ending with the words '-start' and '-end' for single-element directives. BREAKING CHANGE: Directives which previously depended on the implicit grouping between directive-start and directive-end attributes must be refactored in order to see this same behaviour. Before: ``` <div data-fancy-directive-start>{{start}}</div> <p>Grouped content</p> <div data-fancy-directive-end>{{end}}</div> .directive('fancyDirective', function() { return { link: angular.noop }; }) ``` After: ``` <div data-fancy-directive-start>{{start}}</div> <p>Grouped content</p> <div data-fancy-directive-end>{{end}}</div> .directive('fancyDirective', function() { return { multiElement: true, // Explicitly mark as a multi-element directive. link: angular.noop }; }) ```

Directives which expect to make use of the multi-element grouping feature introduced in 1.1.6 (angular@e46100f7) must now add the property multiElement to their definition object, with a truthy value. This enables the use of directive attributes ending with the words '-start' and '-end' for single-element directives. BREAKING CHANGE: Directives which previously depended on the implicit grouping between directive-start and directive-end attributes must be refactored in order to see this same behaviour. Before: ``` <div data-fancy-directive-start>{{start}}</div> <p>Grouped content</p> <div data-fancy-directive-end>{{end}}</div> .directive('fancyDirective', function() { return { link: angular.noop }; }) ``` After: ``` <div data-fancy-directive-start>{{start}}</div> <p>Grouped content</p> <div data-fancy-directive-end>{{end}}</div> .directive('fancyDirective', function() { return { multiElement: true, // Explicitly mark as a multi-element directive. link: angular.noop }; }) ``` Closes angular#5372 Closes angular#6574 Closes angular#5370 Closes angular#8044 Closes angular#7336

ghost assigned jeffbcross Dec 11, 2013

jeffbcross mentioned this pull request Dec 11, 2013

Directive scope variables using hungarian type notation #5370

Closed

rodyhaddad mentioned this pull request Jan 11, 2014

directive-start, directive-end syntax is incompatible with ngController. #5724

Closed

caitp modified the milestones: 1.3.x, Backlog Mar 6, 2014

IgorMinar reviewed Mar 6, 2014
View reviewed changes

caitp reviewed Mar 6, 2014
View reviewed changes

caitp mentioned this pull request May 2, 2014

Custom directives with names ending "Start" or "End" will silently fail to compile #7336

Closed

Schweigi mentioned this pull request May 21, 2014

Implements #53. autoExpand now takes 'both', 'left', 'right', or 'none' angular-gantt/angular-gantt#57

Merged

IgorMinar modified the milestones: 1.3.0-beta.15, 1.3.0 Jul 2, 2014

IgorMinar assigned caitp Jul 2, 2014

ealtenho modified the milestones: 1.3.0-beta.16, 1.3.0-beta.15 Jul 11, 2014

caitp closed this in e8066c4 Jul 16, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($compile): explicitly request multi-element directive behaviour #5372

feat($compile): explicitly request multi-element directive behaviour #5372

caitp commented Dec 11, 2013

jeffbcross commented Dec 11, 2013

caitp commented Dec 11, 2013

rodyhaddad commented Dec 12, 2013

caitp commented Dec 12, 2013

rodyhaddad commented Dec 19, 2013

redaemn commented Dec 19, 2013

gsklee commented Jan 11, 2014

caitp commented Jan 11, 2014

burkeholland commented Feb 17, 2014

IgorMinar Mar 6, 2014

IgorMinar commented Mar 6, 2014

caitp Mar 6, 2014

IgorMinar commented Jul 2, 2014

caitp commented Jul 3, 2014

feat($compile): explicitly request multi-element directive behaviour #5372

feat($compile): explicitly request multi-element directive behaviour #5372

Conversation

caitp commented Dec 11, 2013

jeffbcross commented Dec 11, 2013

caitp commented Dec 11, 2013

rodyhaddad commented Dec 12, 2013

caitp commented Dec 12, 2013

rodyhaddad commented Dec 19, 2013

redaemn commented Dec 19, 2013

gsklee commented Jan 11, 2014

caitp commented Jan 11, 2014

burkeholland commented Feb 17, 2014

IgorMinar Mar 6, 2014

Choose a reason for hiding this comment

IgorMinar commented Mar 6, 2014

caitp Mar 6, 2014

Choose a reason for hiding this comment

IgorMinar commented Jul 2, 2014

caitp commented Jul 3, 2014