Transclude type multi-element, use this for ng-repeat #2531

lgalfaso · 2013-04-27T20:56:25Z

The patch adds a new transclude type multi-element to be able to handle transclusions that include more elements that the one with the directive. The transclusion type multi-element works in the following way.

When a directive is defined of the type multi-element, then Angular looks for matching attributes named "-start" and -end" and transcludes the entire block. If element where the directive is present does not contain the attribute "-start" then the transclusion works just as the transclusion type "element"

E.g

The directive ng-repeat is of type multi-element when compiling the template below, the compiler will look for the attribute ng-repeat-start. Given that this is present, it will look for a matching hg-repeat-end attribute. When found the entire block will be used for the linking function

<ul>
  <li ng-repeat-start ng-repeat="item in items">{{item.name}};</li>
  <li>{{item.number}};</li>
  <li ng-repeat-end>{{item.color}};</li>
</ul>

feat(ng-repeat): Make ng-repeat of transclude type multi-element Add a new transclude type multi-element to be able to handle transclusion that includes more elements than the one where the directive is present Make the changes to ngRepeat so it is of transclude type multi-element Closes angular#1891

Remove a tab from the code

Fix an issue with nested multi-element directives that are siblings at the DOM level

IgorMinar · 2013-05-03T08:42:16Z

I skimmed through this PR and it looks really good! Some feedback:

instead of ngRepeatStartSpec.js, can you add the tests to ngRepeatSpec.js
instead of copying all tests from ngRepeat and converting them to mutli-element repeaters, can you create just one test that will prove that ngRepeat supports multi-element transclusion and one for nested multi-element repeater?
can you please add a whole bunch of tests into compileSpec.js that will test the internals of multi-element transclusion as well as error handling? please see tests in my failed attempt to implement this feature via comment directive: https://github.com/IgorMinar/angular.js/blob/cbc2da279ced543db63cc96c4ab16caace45cf81/test/ng/directive/ngRepeatSpec.js#L846-L899

keep up the good work!

petebacondarwin · 2013-05-03T19:19:23Z

src/ng/compile.js

@@ -418,9 +419,12 @@ function $CompileProvider($provide) {
     *        rootElement must be set the jqLite collection of the compile root. This is
     *        needed so that the jqLite collection items can be replaced with widgets.
     * @param {number=} max directive priority
+     * @param {boolean=} if the max priority should only apply to the first element in the list.


typo:

@param {boolean=} if the max ...

should be:

@param {boolean=} limitMaxPriorityToFirstElement if the max ...

Also, I think that it would be useful to explain why this parameter has been added.

Will make the change to @param but I am not sure about adding the full explanation on why this new parameter was added. Just for completeness, it would be something like this

When extracting a multi-element block, we need to remove the block and compile it to create the linking function for the entire block. Now, before the patch, then calling this compile function, sometimes we call it with one element and a maxPriotity (this was for transclusion = 'element') or with multiple elements and no maxPriotity. With multi-element transclusion, we need to handle another case, this is to get multiple elements but there should be a limit on the priority but just to the first element.

Note: In a strict sense it should be possible to remove this parameter and make the assumption that the maxPriority only applies to the first element when nodeList has multiple elements, but I think it makes clearer that, even when there is some redundancy

petebacondarwin · 2013-05-03T20:20:42Z

I am a bit worried about the animation stuff. I think the ng-repeat directive as it stands doesn't take into account that the template now may contain multiple elements, which all need to be animated.

@petebacondarwin

Avoid cloning elements during a multi-element transclude Put only one comment that will be sent to the linking function and remove all the placeholde elements Cleanup the containsAttr method with an implementation from @petebacondarwin Fixes on the unit tests to follow angular#2492 Document the transclude type 'multi-element' at the docs Fix a typo on the method compileNodes Move the important unit tests from ngRepeatStartSpec to ngRepeatSpec Add more tests at compile for the new multi-element transclude

lgalfaso · 2013-05-04T22:41:37Z

Merge from trunk to get the modification on ngRepat for #2492
Implement the changes as requested from @IgorMinar and @petebacondarwin
Document the transclude type 'multi-element'

mhevery · 2013-05-06T05:50:00Z

I think as it stands now it would allow weird things like this:

<div a a-start></div>
<div b b-start></div>
<div a-end></div>
</div b-end></div>

This of course is non-sense. I think the grouping should not be associated with any one directive so what we really want is this

<div ng-repeat ng-with-next></div>
<div ng-with-next></div>
<div></div>

Also the grouping should make it clear that any directives define on the ng-with-next can only be accessed after transclusion. Here the ng-with-next is pseudo directive for the compiler not a real directives.

Also I don't think that directives should allow for special multi-transclude. There is only one transclude, in some cases it just happens to have multiple elements. So it is not something which needs to be specified explicitly.

What do you guys think of this?

lgalfaso · 2013-05-06T14:08:52Z

@mhevery It should not allow

<div a a-start></div>
<div b b-start></div>
<div a-end></div>
<div b-end></div>

as the compilation process will do the following:

detect the directive a and the attribute a-start
extract the block of elements from a-start to a-end

<div a a-start></div>
<div b b-start></div>
<div a-end></div>

put these elements into a new <div></div>
recurse a compile call into the content of this div
when performing the recursion and processing b with b-start, it will not find a b-end and throw an exception

The reason on why a new transclude type multi-element makes it sure that the directive is able to handle the fact that the linking function can add more than one element. In most cases, this is just a few changes (as it was the case for ng-repeat), but without this, it can break a lot of existing directives that are not ready for this

Fix for the extended `remove` function that is used during `multi-element` transclusion to also remove the element from its parent when there is one

petebacondarwin · 2013-05-06T17:57:59Z

@mhevery - the main issue that I see with ng-with-next is that it doesn't support nesting of ng-repeat. Also, I find it less intuitive and a bit verbose if you have more than two elements to transclude. I generally agree with @lgalfaso's comments.

petebacondarwin · 2013-05-06T18:02:17Z

@mhevery - it would be beautiful if we could make it so that any directive that transcludes can support multiple top level nodes in it transclusion (something like ng-transclude-start, ng-transclude-end) but as @lgalfaso points out this would require lots of directives to change the way that they handle transclusion - i.e. they would need to know that they need to move or remove multiple elements.

lgalfaso · 2013-05-08T14:57:46Z

Even when I think that the new transclude type multi-element would be a fine addition, it looks like it will not solve all the issues with ng-repeat
Here are two examples (that have the same limitation).

Keep in mind that the templates are not valid, but are just examples of issues

Nested repeats that start at the same element

<dl>
  <dt ng-repeat="group in groups" ng-repeat-start ng-repeat="item in group">{{item}}</dt>
  <dd ng-repeat-end>{{group}}</dd>
</dl>

Nested repeats that end at the same element

<dl>
  <dt ng-repeat="group in groups" ng-repeat-start>{{group}}</dt>
  <dd ng-repeat="item in group" ng-repeat-start>{{item.id}}</dd>
  <dd ng-repeat-end ng-repeat-end>{{item.name}}</dd>
</dl>

Now, in a strict sense, I think this is a limitation on ng-repeat (and for sure, something that should not be part of this PR)

If this PR moves forward, will look into how to solve these issues

petebacondarwin · 2013-05-10T12:14:20Z

@lgalfaso - multiple ng-repeats on a single element are not very reliable even now, so I wouldn't worry about this.
http://plnkr.co/edit/ciKUFzlydMwUWoCOXKnM?p=preview

petebacondarwin · 2013-05-10T12:46:22Z

petebacondarwin · 2013-05-10T13:08:32Z

@lgalfaso - this will need to be rebased onto master since the animation css class names have changed.

petebacondarwin · 2013-05-10T13:13:16Z

Currently failing to build: http://ci.angularjs.org/job/angular.js-pete/153/console

petebacondarwin · 2013-05-10T13:29:11Z

Getting better - after a few tweaks - but of course Internet Explorer is complaining: http://ci.angularjs.org/job/angular.js-pete/154/console

style($compile): clarify argument name Merge branch 'master' into ng-repeat-start Conflicts: test/ng/directive/ngRepeatSpec.js

lgalfaso · 2013-05-10T13:58:43Z

@petebacondarwin I do not have IE8 to test this, would it be possible to know if 6861142 passed at IE8?

petebacondarwin · 2013-05-10T15:45:58Z

Here you go...
http://ci.angularjs.org/job/angular.js-pete/155/console

lgalfaso · 2013-05-10T15:48:23Z

Ok, let me look for an IE8 box image and figure this out

Fix for [].splice on IE8 that has the second argument as mandatory Fix for arrays on IE8 that adds an extra element on trailing comma

lgalfaso · 2013-05-10T17:51:14Z

@petebacondarwin 0fae109 fixes the IE8 issues

IgorMinar · 2013-05-17T05:55:12Z

src/ng/compile.js

+          transcludeContent.push(cursor);
+          cursor = cursor.nextSibling;
+        } while(count > 0 && cursor);
+        if (count > 0) throw Error('Unmatched ' + transcludeStart + '.');


it should be throw new Error...

without new the stack trace will contain an extra frame which looks confusing.

Add a `new` when throwing an error

lgalfaso · 2013-05-17T14:48:29Z

Thanks @IgorMinar added the new when throwing these errors

mhevery · 2013-05-24T19:44:25Z

This PR is now superseded by this one: #2783

Very good idea, I have built upon your idea and implemented a more generic version.

Closing

lgalfaso added 3 commits April 27, 2013 17:37

style(compile): Remove a tab

477ae0c

Remove a tab from the code

fix(compile): Fix for nested multi-element directives

6861142

Fix an issue with nested multi-element directives that are siblings at the DOM level

petebacondarwin reviewed May 3, 2013
View reviewed changes

lgalfaso added 2 commits May 4, 2013 17:32

Merge remote-tracking branch 'upstream/master' into ng-repeat-start

efd5bd7

fix(compile): Fix for extended remove function

95d8999

Fix for the extended `remove` function that is used during `multi-element` transclusion to also remove the element from its parent when there is one

chore(ngAnimate): CSS classes X-setup/X-start -> X/X-active

9692381

style($compile): clarify argument name Merge branch 'master' into ng-repeat-start Conflicts: test/ng/directive/ngRepeatSpec.js

fix(compile): IE8 specific fix

0fae109

Fix for [].splice on IE8 that has the second argument as mandatory Fix for arrays on IE8 that adds an extra element on trailing comma

IgorMinar reviewed May 17, 2013
View reviewed changes

fix(compile): add new when throwing an error

9af7cb4

Add a `new` when throwing an error

ghost assigned mhevery May 23, 2013

mhevery closed this May 24, 2013

rodyhaddad mentioned this pull request Jul 29, 2013

feat($compile): support multi-elements in all declaration styles #3379

Closed

rodyhaddad mentioned this pull request Oct 9, 2013

feat(ngRepeat): use block separator comments #4157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transclude type multi-element, use this for ng-repeat #2531

Transclude type multi-element, use this for ng-repeat #2531

lgalfaso commented Apr 27, 2013

IgorMinar commented May 3, 2013

petebacondarwin May 3, 2013

lgalfaso May 4, 2013

petebacondarwin commented May 3, 2013

lgalfaso commented May 4, 2013

mhevery commented May 6, 2013

lgalfaso commented May 6, 2013

petebacondarwin commented May 6, 2013

petebacondarwin commented May 6, 2013

lgalfaso commented May 8, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

lgalfaso commented May 10, 2013

petebacondarwin commented May 10, 2013

lgalfaso commented May 10, 2013

lgalfaso commented May 10, 2013

IgorMinar May 17, 2013

lgalfaso commented May 17, 2013

mhevery commented May 24, 2013

Transclude type multi-element, use this for ng-repeat #2531

Transclude type multi-element, use this for ng-repeat #2531

Conversation

lgalfaso commented Apr 27, 2013

IgorMinar commented May 3, 2013

petebacondarwin May 3, 2013

Choose a reason for hiding this comment

lgalfaso May 4, 2013

Choose a reason for hiding this comment

petebacondarwin commented May 3, 2013

lgalfaso commented May 4, 2013

mhevery commented May 6, 2013

lgalfaso commented May 6, 2013

petebacondarwin commented May 6, 2013

petebacondarwin commented May 6, 2013

lgalfaso commented May 8, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

petebacondarwin commented May 10, 2013

lgalfaso commented May 10, 2013

petebacondarwin commented May 10, 2013

lgalfaso commented May 10, 2013

lgalfaso commented May 10, 2013

IgorMinar May 17, 2013

Choose a reason for hiding this comment

lgalfaso commented May 17, 2013

mhevery commented May 24, 2013