Nested transclude issue #7387
Nested transclude issue #7387
Conversation
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
|
I have added more tests when we have async templates and tidied up @vojtajina's fix. |
| expect(element.text().trim()).toEqual('transcluded content'); | ||
| })); | ||
|
|
||
| it('should allow nested transclude directives with sync templates', inject(function($compile, $rootScope) { |
There was a problem hiding this comment.
shouldn't we also create the same set of tests for async directives?
There was a problem hiding this comment.
I agree, they are two separate code paths and the other needs tests as well
There was a problem hiding this comment.
Hold on! That is exactly what these tests do they were just badly named :-)
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Fixes angular#7240 Closes angular#7387
| @@ -1346,7 +1349,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| } | |||
|
|
|||
| nodeLinkFn.scope = newScopeDirective && newScopeDirective.scope === true; | |||
| nodeLinkFn.transclude = hasTranscludeDirective && childTranscludeFn; | |||
There was a problem hiding this comment.
@petebacondarwin shouldn't this depend on whether there is a template directive or not?
If there is a template directive, then we don't pass the transclusion, right?
At least that was what we came up with, I think...
vojtajina@e10bd88#diff-a732922b631efed1b9f33a24082ae0dbR1335
There was a problem hiding this comment.
I moved this check further up the diff
There was a problem hiding this comment.
@vojtajina - sorry about the short answer I was on my phone.
I looked through the logic and I am pretty sure that the logic here works out the same as our original diff.
There was a problem hiding this comment.
if (hasTranscludeDirective) {
nodeLinkFn.transcludeOnThisElement = true;
} else if (!templateDirective) {
nodeLinkFn.transcludeOnThisElement = false;
}
is equivalent to
nodeLinkFn.transcludeOnThisElement = hasTranscludeElement
since the default of this property is undefined (i.e. falsy)
There was a problem hiding this comment.
The original code does not set nodeLinkFn.transclude if !hasTransclude && templateDirective.
There was a problem hiding this comment.
@petebacondarwin Oh, thanks. Now I understand that the code does the same thing. You are right.
I'm still however not sure if that's correct, as template SHOULD be deciding factor, no?
There was a problem hiding this comment.
I mean, I understand that the code change you did, does not change any behavior, but I'm still questioning whether it is the correct behavior.
There was a problem hiding this comment.
Yes I am willing to agree
There was a problem hiding this comment.
This is what I was talking about. I think this failing test shows the problem:
vojtajina@4d152c8
There was a problem hiding this comment.
We can fix that in a separate PR though.
|
Hmm possibly...
|
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes #7240 Closes #7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes #7240 Closes #7387
|
@petebacondarwin: I'm still experiencing issues with nested transcludes in version 1.3.0-rc.0. JSFiddle example here – in the second test, the nested transclude is ignored. It seems to lose the transcluded content when the ng-transclude is applied to the same element as the nested directive within the outer directive's template. It works if the ng-transclude is applied to a wrapper element inside the nested directive (but then you're left with an irritating wrapper element). Is this the same issue? |
|
@timkendrick, it's a similar but different issue, so I reposted your comment as #8914 |
|
Yes, @timkendrick 's fiddle shows a valid issue. Just not sure how high priority this should be as there is a simple work-around. |
|
Thanks for the quick response guys! |
Solves the problem of transclude directives that contain transclude directives in their template.
Fixes #7240
Fixes #4969