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 3 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
80 changes: 76 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,21 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* 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 +650,24 @@ function $CompileProvider($provide) {
compileNode = $compileNode[0];
replaceWith($rootElement, 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>');
forEach(nestedContent, function(nestedElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that you clone these nodes for the template but then replace the originals with comments later anyway?
Wouldn't it be simpler and faster just to move them all into the template directly and just put one comment in their place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is possible, will make the change and post a new version of the patch

cloneContent.append(JQLiteClone(nestedElement));
});
childTranscludeFn = compile(cloneContent.contents(), transcludeFn, terminalPriority, true);
$template = jqLite(compileNode);
$compileNode = templateAttrs.$$element =
jqLite(document.createComment(' ' + directiveName + ': ' + templateAttrs[directiveName] + ' '));
compileNode = $compileNode[0];
replaceWith($rootElement, $template, compileNode);
forEach(nestedContent.splice(1),
function(toRemove) {
replaceWith($rootElement, jqLite(toRemove), document.createComment(' placeholder '));
}
);
} else {
$template = jqLite(JQLiteClone(compileNode)).contents();
$compileNode.html(''); // clear contents
Expand Down Expand Up @@ -731,6 +754,55 @@ 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 nodeType = node.nodeType,
result = false;

switch(nodeType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any need to have this as a switch statement, given that there is only one clause?

if ( node.nodeType == 1 ) {

Also means that you can ditch var nodeType = node.nodeType

case 1:
// iterate over the attributes
for (var attr, name, nName, ngAttrName, nAttrs = node.attributes,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this block of code was mostly copied from collectDirectives(). It can be simplified...

j = 0, jj = nAttrs && nAttrs.length; j < jj; j++) {
attr = nAttrs[j];
if (attr.specified) {
name = attr.name;
// support ngAttr attribute binding
ngAttrName = directiveNormalize(name);
if (NG_ATTR_BINDING.test(ngAttrName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ng-attr- stuff is not relevant here. So these three lines can be removed.

name = ngAttrName.substr(6).toLowerCase();
}
nName = directiveNormalize(name.toLowerCase());
if (nName == attributeName) {
result = true;
break;
}
}
}
break;
}
return result;
}

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
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
Loading