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

feat(ngRepeat): use block separator comments #4157

Closed
wants to merge 1 commit into from

Conversation

jankuca
Copy link
Contributor

@jankuca jankuca commented Sep 25, 2013

This change allows the use of ngRepeat in combination with directives that use transclusion and directives that replace its original elements or add more elements to ngRepeat blocks.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@jeffbcross
Copy link
Contributor

Tests passed on CI: http://ci.angularjs.org/job/angular.js-jeff/58/console

@jeffbcross
Copy link
Contributor

Can you change the commit message to close the issues that this resolves? https://help.github.com/articles/closing-issues-via-commit-messages

Unfortunately, I tried applying the fix to issue #4002 but it didn't completely fix it. I think it may require a separate PR though. If you want to see the failing example, see this plnkr: http://plnkr.co/edit/Hjqpqbuy0nhrTOmCjk36?p=preview and swap out the angular.js src with http://ci.angularjs.org/job/angular.js-jeff/58/artifact/build/angular.js

@rodyhaddad
Copy link
Contributor

There is a problem with multi-element directives: if they contain an element with a directive having transclude: 'element', they will fail to remove any node that that directive add:

<div ng-if-start="expr"></div>
<div ng-repeat="item in list">{{ item }}</div>
<div ng-if-end></div>

Anything that ngRepeat adds in the example above won't be removed by ngIf when expr evaluates to false

This PR solves the problem for ngRepeat, which is where multi-element directives are used the most. But a more generic solution implemented in $compile would be much better in the long run.

@ghost ghost assigned IgorMinar and vojtajina Oct 2, 2013
var children = element.find('div');
expect(children.length).toBe(3);

// Note: COMMENT_NODE === 8
Copy link
Contributor

Choose a reason for hiding this comment

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

better to create a DSL, toBeComment()

@vojtajina
Copy link
Contributor

I added some changes in https://github.com/vojtajina/angular.js/tree/pr-4157:

  • take care of moving elements too
  • remove block.elements as we don't use it anymore

@IgorMinar can you review that ?

@rodyhaddad
Copy link
Contributor

While this does solve #3104, #3104 unveils an underlying problem that the changes in this PR don't address (and I tried to explain the problem in that issue a while ago)

This only works for ngRepeat, it's simply not generic!
Any directive that gets used in a multi-element fashion will face the exact same problem.
That's why it should be baked in $compile (or something similar), instead of having the individual directive solve the problem on its own.

It's like the whole multi-element implementation scenario again: The first PRs submitted on the subject worked just for ngRepeat (#1646, #2531), but then we had a PR (#2783) that made $compile support multi-element directives natively with -start/-end, and that's awesome because others can benefit from the progress.

I'm not saying that this PR is bad: if we're looking for a solution to include in the 1.2.0rc3 release, then this is great. But in the long run, it would be better to solve the problem in a way that will work for all directives, not just one. We will surely get new issues when people start using -start/-end with more than just ngRepeat.

@vojtajina
Copy link
Contributor

@rodyhaddad you are right - $compile should take care of this in order to make it more general. That would be however a much bigger change, so for 1.2 I'm merging this fix. The future version of Angular will more likely have a re-designed "compiler" (similar to what angular.dart has now), that takes care of this stuff.

@vojtajina
Copy link
Contributor

Landed as 9efa46a.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants