fix(ngMessages): ensure that multi-level transclusion works with ngMessagesInclude #11199
Conversation
matsko
force-pushed
the
fix_pr_11196
branch
2 times, most recently
from
3200e3b to
68d5bef
Compare
| link: function($scope, element, attrs) { | ||
| $templateRequest(attrs.ngMessagesInclude || attrs.src).then(function(html) { | ||
| element.html(html); | ||
| $compile(element.contents())($scope); |
There was a problem hiding this comment.
I'm not sure this is the best approach. Why not just make use of the clone attach feature of the link function?
$templateRequest(attrs.ngMessagesInclude || attrs.src).then(function(html) {
$compile(html)($scope, function(dom) {
// attach to DOM using your favourite method --- finding the parent ngMessages directive will no longer fail.
});
There was a problem hiding this comment.
What's the benefit to having this code be inserted in a different way? There's no animation stuff here since that's managed in the <ng-message> directive level which transcludes on its own anyway.
There was a problem hiding this comment.
the benefit is not adding a breaking change --- keeping the DOM looking the way it did before
There was a problem hiding this comment.
That works amazing! Thanks! You just saved a very inconvenient breaking change.
matsko
changed the title
matsko
changed the title
|
LGTM, but I would suggest adding some comments explaining the flow and assertions in that large unit test. |
| cursor = elm; | ||
| }); | ||
| $compile(elements)($scope); | ||
| require: '^^ngMessages', // we only require this for validation sake |
There was a problem hiding this comment.
actually, hang on. Previously, you were basically replacing the element with a comment placeholder. I think we should keep doing this, there's no good reason to keep it in the DOM (is there?)
There was a problem hiding this comment.
All fixed up now. And I added some comments to the unit test.
…ssagesInclude ngRepeat and any other directives that alter the DOM structure using transclusion may cause ngMessagesInclude to behave in an unpredictable manner. This fix ensures that the element containing the ngMessagesInclude directive will stay in the DOM to avoid these issues. Closes angular#11196
|
thanks, lgtm. |
|
Merged as d7ec5f3 |
|
Thanks @caitp |
ngRepeat and any other directives that alter the DOM structure using
transclusion may cause ngMessagesInclude to behave in an unpredictable
manner. This fix ensures that the element containing the ngMessagesInclude
directive will stay in the DOM to avoid these issues.
Closes #11196