Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fca21fe
feat(compile): Add transclude type "multi-element"
lgalfaso Apr 27, 2013
477ae0c
style(compile): Remove a tab
lgalfaso Apr 27, 2013
6861142
fix(compile): Fix for nested multi-element directives
lgalfaso Apr 28, 2013
efd5bd7
Merge remote-tracking branch 'upstream/master' into ng-repeat-start
lgalfaso May 4, 2013
130fd03
refactor(compile): Optimizations for the directive 'multi-element'
lgalfaso May 4, 2013
95d8999
fix(compile): Fix for extended `remove` function
lgalfaso May 6, 2013
9692381
chore(ngAnimate): CSS classes X-setup/X-start -> X/X-active
lgalfaso May 10, 2013
0fae109
fix(compile): IE8 specific fix
lgalfaso May 10, 2013
9af7cb4
fix(compile): add `new` when throwing an error
lgalfaso May 17, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/content/guide/directive.ngdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,11 @@ compiler}. The attributes are:

* `true` - transclude the content of the directive.
* `'element'` - transclude the whole element including any directives defined at lower priority.

* `'multi-element'` - if the node where this directive is present contains an attribute
named `${directiveName}-start` (were `${directiveName}` is the name of the directive) then
the entire block of elements is transcluded until a sibling node is found with and attribute
named `${directiveName}-end`. If and attribute name `${directiveName}-start` is not found
then it behaves like the `'element'` transclude.

* `compile`: This is the compile function described in the section below.

Expand Down
82 changes: 78 additions & 4 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ function $CompileProvider($provide) {

//================================

function compile($compileNodes, transcludeFn, maxPriority) {
function compile($compileNodes, transcludeFn, maxPriority, limitMaxPriorityToFirstElement) {
if (!($compileNodes instanceof jqLite)) {
// jquery always rewraps, whereas we need to preserve the original selector so that we can modify it.
$compileNodes = jqLite($compileNodes);
Expand All @@ -369,7 +369,8 @@ function $CompileProvider($provide) {
$compileNodes[index] = jqLite(node).wrap('<span></span>').parent()[0];
}
});
var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority);
var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority,
limitMaxPriorityToFirstElement);
return function publicLinkFn(scope, cloneConnectFn){
assertArg(scope, 'scope');
// important!!: we must call our jqLite.clone() since the jQuery one is trying to be smart
Expand Down Expand Up @@ -418,17 +419,22 @@ 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=} limitMaxPriorityToFirstElement if the max priority should only apply to
* the first element in the list. A true value here will make the maxPriority only apply
* to the first element on the list while the other elements on the list will not have
* a maxPriority set
* @returns {?function} A composite linking function of all of the matched directives or null.
*/
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority) {
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, limitMaxPriorityToFirstElement) {
var linkFns = [],
nodeLinkFn, childLinkFn, directives, attrs, linkFnFound;

for(var i = 0; i < nodeList.length; i++) {
attrs = new Attributes();

// we must always refer to nodeList[i] since the nodes can be replaced underneath us.
directives = collectDirectives(nodeList[i], [], attrs, maxPriority);
directives = collectDirectives(nodeList[i], [], attrs,
(limitMaxPriorityToFirstElement && i != 0) ? undefined : maxPriority);

nodeLinkFn = (directives.length)
? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement)
Expand Down Expand Up @@ -645,6 +651,24 @@ function $CompileProvider($provide) {
compileNode = $compileNode[0];
replaceWith(jqCollection, jqLite($template[0]), compileNode);
childTranscludeFn = compile($template, transcludeFn, terminalPriority);
} else if (directiveValue == 'multi-element') {
// We need to compile a clone of the elements, but at the same time we have to be sure that these elements are siblings
var nestedContent = extractMultiElementTransclude(compileNode, directiveName);
var cloneContent = jqLite('<div></div>');

$template = jqLite(compileNode);
$compileNode = templateAttrs.$$element =
jqLite(document.createComment(' ' + directiveName + ': ' + templateAttrs[directiveName] + ' '));
compileNode = $compileNode[0];
replaceWith(jqCollection, jqLite($template[0]), compileNode);
cloneContent.append($template);
forEach(nestedContent.splice(1, nestedContent.length - 1),
function(toRemove) {
removeElement(jqCollection, toRemove);
cloneContent.append(toRemove);
}
);
childTranscludeFn = compile(cloneContent.contents(), transcludeFn, terminalPriority, true);
} else {
$template = jqLite(JQLiteClone(compileNode)).contents();
$compileNode.html(''); // clear contents
Expand Down Expand Up @@ -731,6 +755,39 @@ function $CompileProvider($provide) {

////////////////////


function extractMultiElementTransclude(cursor, directiveName) {
var transcludeContent = [],
c, count = 0,
transcludeStart = directiveName + 'Start',
transcludeEnd = directiveName + 'End';

do {
if (containsAttr(cursor, transcludeStart)) count++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do our coding conventions allow single line if statements without {} braces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was not aware of this. In fact, I want to follow the "implicit" coding convention that, if the action is simple, then do not put braces

$ grep " if (" src/ -r | grep -v \{ | grep \; | wc -l
     177

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do occasionally use this style as long as the whole if block is a one-liner.

if (containsAttr(cursor, transcludeEnd)) count--;
transcludeContent.push(cursor);
cursor = cursor.nextSibling;
} while(count > 0 && cursor);
if (count > 0) throw Error('Unmatched ' + transcludeStart + '.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be throw new Error...

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

if (count < 0) throw Error('Unexpected ' + transcludeEnd + '.');
return transcludeContent;
}


function containsAttr(node, attributeName) {
var attr, attrs = node.attributes, length = attrs && attrs.length;
if ( length ) {
for (var j = 0; j < length; j++) {
attr = attrs[j];
if (attr.specified && directiveNormalize(attr.name) == attributeName) {
return true;
}
}
}
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I would suggest that this whole function could be replaced with:

function containsAttr(node, attributeName) {
  var attr, attrs = node.attributes, length = attrs && attrs.length;
  if ( length ) {
    for (var j = 0; j < length; j++) {
      attr = attrs[j];
      if (attr.specified && directiveNormalize(attr.name) == attributeName) {
        return true;
      }
    }
  }
  return false;
}

But I might be missing something :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks!


function addLinkFns(pre, post) {
if (pre) {
pre.require = directive.require;
Expand Down Expand Up @@ -1152,6 +1209,23 @@ function $CompileProvider($provide) {
newNode[jqLite.expando] = oldNode[jqLite.expando];
$element[0] = newNode;
}


function removeElement($rootElement, element) {
var i, ii, parent = element.parentNode;

if ($rootElement) {
for(i = 0, ii = $rootElement.length; i < ii; i++) {
if ($rootElement[i] == element) {
$rootElement.splice(i, 1);
break;
}
}
}
if (parent) {
parent.removeChild(element);
}
}
}];
}

Expand Down
10 changes: 5 additions & 5 deletions src/ng/directive/ngRepeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
var ngRepeatDirective = ['$parse', '$animator', function($parse, $animator) {
var NG_REMOVED = '$$NG_REMOVED';
return {
transclude: 'element',
transclude: 'multi-element',
priority: 1000,
terminal: true,
compile: function(element, attr, linker) {
Expand Down Expand Up @@ -258,7 +258,7 @@ var ngRepeatDirective = ['$parse', '$animator', function($parse, $animator) {
if (lastBlockMap.hasOwnProperty(key)) {
block = lastBlockMap[key];
animate.leave(block.element);
block.element[0][NG_REMOVED] = true;
forEach(block.element, function(leavingElement) { leavingElement[NG_REMOVED] = true; });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't be needed if the repeat template was replaced with only one comment - am I missing something important here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is marking all the elements in the multi-element block that they will be removed.
Will make the change so the transclusion only puts one comment (and not one comment per element that was removed), but this needs to stay as it is

block.scope.$destroy();
}
}
Expand All @@ -274,7 +274,7 @@ var ngRepeatDirective = ['$parse', '$animator', function($parse, $animator) {
// associated scope/element
childScope = block.scope;

nextCursor = cursor[0];
nextCursor = cursor[cursor.length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we need the cursor to be at the last element on the previous multi-element block

do {
nextCursor = nextCursor.nextSibling;
} while(nextCursor && nextCursor[NG_REMOVED]);
Expand All @@ -284,7 +284,7 @@ var ngRepeatDirective = ['$parse', '$animator', function($parse, $animator) {
cursor = block.element;
} else {
// existing item which got moved
animate.move(block.element, null, cursor);
animate.move(block.element, null, cursor.eq(-1));
cursor = block.element;
}
} else {
Expand All @@ -301,7 +301,7 @@ var ngRepeatDirective = ['$parse', '$animator', function($parse, $animator) {

if (!block.element) {
linker(childScope, function(clone) {
animate.enter(clone, null, cursor);
animate.enter(clone, null, cursor.eq(-1));
cursor = clone;
block.scope = childScope;
block.element = clone;
Expand Down
93 changes: 93 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,46 @@ describe('$compile', function() {
})
});

it("should complain when the multi-element end tag can't be found among one of the following siblings", inject(function($compile) {
forEach([
// no content, no end tag
'<div ng-repeat="item in items" ng-repeat-start></div>',

// content, no end tag
'<div ng-repeat="item in items" ng-repeat-start></div>' +
'<span>{{item.text}}></span>' +
'<span>{{item.done}}</span>',

// content, end tag too deep
'<div ng-repeat="item in items" ng-repeat-start></div>' +
'<div>' +
'<span>{{item.text}}></span>' +
'<span>{{item.done}}</span>' +
'<span ng-repeat-end></span>' +
'</div>',

// content, end tag too high
'<div>' +
'<div ng-repeat="item in items" ng-repeat-start></div>' +
'<span>{{item.text}}></span>' +
'<span>{{item.done}}</span>' +
'</div>' +
'<div ng-repeat-end></div>'
], function(template) {
expect(function() {
$compile('<div>' + template + '</div>');
}).toThrow("Unmatched ngRepeatStart.");
});
}));


it("should complain when there is an end attribute at the start of a multi-element directive", inject(function($compile) {
expect(function() {
$compile('<div><li ng-repeat-end ng-repeat="i in items"></li><li></li></div>');
}).toThrow("Unexpected ngRepeatEnd.");
}));


describe('compiler control', function() {
describe('priority', function() {
it('should honor priority', inject(function($compile, $rootScope, log){
Expand Down Expand Up @@ -2345,6 +2385,35 @@ describe('$compile', function() {
});


it('should compile get templateFn on multi-element', function() {
module(function() {
directive('trans', function(log) {
return {
transclude: 'multi-element',
priority: 2,
controller: function($transclude) { this.$transclude = $transclude; },
compile: function(element, attrs, template) {
log('compile: ' + angular.mock.dump(element));
return function(scope, element, attrs, ctrl) {
log('link');
var cursor = element;
template(scope.$new(), function(clone) {cursor.after(clone); cursor = clone.eq(-1);});
ctrl.$transclude(function(clone) {cursor.after(clone)});
};
}
}
});
});
inject(function(log, $rootScope, $compile) {
element = $compile('<div><div high-log trans="text" trans-start log>{{$parent.$id}}-{{$id}};</div><div trans-end>{{$parent.$id}}-{{$id}};</div></div>')
($rootScope);
$rootScope.$apply();
expect(log).toEqual('compile: <!-- trans: text -->; HIGH; link; LOG; LOG');
expect(element.text()).toEqual('001-002;001-002;001-003;001-003;');
});
});


it('should support transclude directive', function() {
module(function() {
directive('trans', function() {
Expand Down Expand Up @@ -2485,6 +2554,30 @@ describe('$compile', function() {
});


it('should support transcluded multi-element on root content', function() {
var comment;
module(function() {
directive('transclude', valueFn({
transclude: 'multi-element',
compile: function(element, attr, linker) {
return function(scope, element, attr) {
comment = element;
};
}
}));
});
inject(function($compile, $rootScope) {
var element = jqLite('<div>before<div transclude transclude-begin></div><div transclude-end></div>after</div>').contents();
expect(element.length).toEqual(4);
expect(nodeName_(element[1])).toBe('DIV');
$compile(element)($rootScope);
expect(nodeName_(element[1])).toBe('#comment');
expect(nodeName_(comment)).toBe('#comment');
expect(nodeName_(element[2])).toBe('DIV');
});
});


it('should safely create transclude comment node and not break with "-->"',
inject(function($rootScope) {
// see: https://github.com/angular/angular.js/issues/1740
Expand Down
Loading