-
Notifications
You must be signed in to change notification settings - Fork 27.5k
Conversation
@IgorMinar - This looks good to me. If you are going to have Also, as an aside, have you thought about the idea of a different kind of repeat directive:
|
LGTM. Can you update ng-repeat documentation and describe the multi-comment as well as IE limitations. |
👍 |
1/ 2/ |
Fair enough about the potential for memory leaks and I appreciate that you don't want to create proliferation of directives that do similar jobs. One last thought: Comment directives that enclose DOM elements. Should this be baked into the $compile - rather than being hard coded into the ng-repeat directive only - so that other directive developers could make use of it? Then you could easily write comment directives for things like ng-switch and so on - and custom directives like ui-if, for instance. |
match; | ||
|
||
// comment-based repeater | ||
if (linker.$$originalNodeType === 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you not move all this into compile.js and allow comment directives to "contain" DOM elements generically?
This would also resolve the issue of needing to pass down the $$originalNodeType into the link function.
@petebacondarwin building this into $compile does make sense. but let's take it one step at a time. I learned quite lot from implementing it as a part of the directive. how would directives declare that they want the compiler to find the ending comment tag, grab everything in between and use that for transclusion? and do that in a way that would not interfere with regular repeater? |
I might be missing something obvious here... I guess that you would need to decide a convention for closing comment tags, which generalised what you have done for ng-repeat. I.E. for some directive my-directive you could have
Then when the compiler spots a comment that matches a directive, which has asked for transclusion, it would simply check all the following siblings for a comment that matched the closing comment format. If so then it would pull those nodes into the transclusion function as though they were child nodes of an element. If your comment directive did not ask for transclusion then it would not look for the closing comment, if it did ask for transclusion and the closing comment is not found then that would be an error. I am sure there are loads of potential faults with this!! |
what about handling pseudo-nested comment directives?
the current repeater doesn't support the case when the "nested" repeater comments are siblings because with repeaters it's very rare to need this. general purpose compiler should however deal with this. |
Ouch. On 19 December 2012 13:11, Igor Minar [email protected] wrote:
|
Obviously, this could be done by keeping a depth count but the trouble here
On 19 December 2012 13:20, Peter Bacon Darwin [email protected] wrote:
|
// search for closing comment tag and create the template | ||
while (sibling) { | ||
if (sibling.nodeType == 8 && | ||
(match = (sibling.textContent || sibling.text).match(NG_REPEAT_END_TAG_REGEXP)) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that sibling.data was a more reliable method for getting the content of the comment - particular with IE8
jqLite('<!-- /ng-repeat -->').data === ' /ng-repeat '
Since you are checking the node type anyway you could simplify the regex and match logic.
repeaters can now be invoked via comments: ``` <table> <!-- directive: ng-repeat item in items --> <tr><td>{{item.text}}</td></tr> <tr><td>something else</td></tr> <!-- /ng-repeat --> </table>
Should this be able to support multiple nested comment repeaters? I realize in the attached scenario I could use divs, but I was trying to support multiple nested repeaters inside of a table where these comments would be required. I thought it might be due to how the browser was generating the dom for the table so I reproduced it outside of a table. I am a little new to open source, but I am willing to help with just a little direction. |
I made some tweaks and made a pull request to support nested comment based repeaters |
Too many IE restrictions and not going the direction of the specs. A better way to do it is to have something like this <div ng-repeat ng-repeat-next</div>
<div> I am part of the repeater</div> |
@mhevery can you edit your last comment to make the code readable...? Thanks. :D |
Is this part of 1.0.7 and 1.1.5? When I try this: http://plnkr.co/edit/pBskLN?p=preview But, change out script reference to the CDN hosted ones, it doesn't work. |
@JogoShugh - the comment-based repeater had too many browser specific issues to overcome. We have solved the problem using the |
There are cases where ng-repeat-start|end isn't that helpful.
Above code has me creating a span element just because. I wanted to repeat the nodes and where a node is active and has childnodes repeat it's child nodes adjacent (not nested) to it. One thing that would have been gold, at least in my scenario was if I could have put the ng-repeat directive on the ul element and told it to repeat the contents of the element rather than the element (and contents) itself. That would have allowed me to do what I wanted without inserting unwanted element. |
@worldspawn You can get rid of "" by adding "ng-repeat-end" to your 2nd |
Hmm that seems to work. Thanks. |
<select>
element, so there is no way to get it working there$$originalNodeType
property on the linking fn that is built by the transclusion. I'm happy to consider other options if someone suggests some.