-
Notifications
You must be signed in to change notification settings - Fork 27.5k
feat($compile): multiple transclusion via named slots #12934
Conversation
This is a WIP and, of course, needs optimizing and documenting. |
Added support for required and optional slots; where the compiler will error if a required slot is not filled. |
@kara, @IgorMinar & @fenduru what do you think of this approach? |
This is more explicit than #12742, which is good. Are there any other major differences? |
@petebacondarwin I like the way this is coming along. After playing with it a little bit, here are a couple of breaking test cases. The first is a bug, and the second is more of desired behavior. Moving the elements around during the compile step means that we're relying on the DOM being available already. This prevents the child elements from being created via directives like Due to #8914, I strongly advocate never using the attribute version of Should the key in DDO's transclude object use One use case I have for this kind of feature would be to ensure proper order of child elements (instead of relying on consumers providing them in the correct order). For instance, I have a directive like: directive('foo', function() {
return {
restrict: 'E',
transclude: {
'one': 'oneSlot',
'two': 'twoSlot',
'three': 'threeSlot'
},
template: '<div ng-transclude="oneSlot"></div>' +
'<div ng-transclude="twoSlot"></div>' +
'<div ng-transclude="threeSlot"></div>'
};
}) So when a consumer writes: <foo>
<two>...</two>
<three>...</three>
<one>...</one>
</foo> they are rendered in the correct order. The problem with this is that this slots implementation requires an additional wrapper around each slot content (since you need an |
// Check for required slots that were not filled | ||
forEach(filledSlots, function(filled, slotName) { | ||
if (!filled) { | ||
throw $compileMinErr('reqslot', 'Required transclusion slot `{0}` was not filled.'); |
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 think we're missing the slotName
here to replace {0}
:
throw $compileMinErr('reqslot', 'Required transclusion slot `{0}` was not filled.', slotName);
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.
Spot on @kara! Thanks
I like this approach. Setting up a transclude map makes sense, and the The only thing still missing for me is the ability to use selectors other than node names, like it would be possible to do with |
@fenduru - thank you for your detailed and helpful response. I took a look at the failing tests you provided. I have added a fix for the bug and I will look into how we can support the second. By the way, you can combine the element |
@kara I think it is reasonable to restrict the things that can be selected, both for performance reasons but also, IMO, to make the whole thing easier to grok when you come across a directive that makes use of this multi-transclusion feature. Once this is in, I would be willing to have discussion about making the selector more general, since that would not be a breaking change to the currently proposed API. |
We could normalise the element name, and store match against the normalized form stored as the key in the |
Do you mean that we should have an Or are you saying that the |
@petebacondarwin are you sure that would work? I would expect the element's link function to run, transcluding all of the non-slot content, then the attribute directive to also run transcluding the "boss" slot as well. Also, I'm saying it would be nice to have something like |
@petebacondarwin That actually only happens to work by chance. Both instances of Using that would break certain directives I've written that register with a parent directive during |
@petebacondarwin I've alluded to a userspace version of slots that I've written. Here is a gist with the approach in case it provides any insight. Couple of key points:
|
By jove @fenduru you are right! I never knew that. I always assumed that if the element matched then we wouldn't match the same on an attribute or class. |
I like your work @fenduru - but I would like to get the feature more baked into the core so that users don't need to use custom helpers and controllers. I will check it out and see if there are useful ideas we can use. I am wondering whether |
@petebacondarwin in my implementation only the child elements have But yeah, I would also love if this was baked in (hence my interest in this PR :P) |
I think (:embarassed:) that $$tlb stands for "transclude late bound". |
649a7cd
to
e0abcdb
Compare
@@ -1472,7 +1472,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
|
|||
function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn) { | |||
|
|||
var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) { | |||
var boundTranscludeFn = function boundTranscludeFn(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) { |
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 just added this name to the function to aid with debugging. It will be stripped out during minification, I believe.
55c1ef7
to
fe4d16b
Compare
This is looking really good! I'm sad to see the removal of the composition test case. Without it, it will be impossible to use this feature when constructing recursive directives. The only way I can think to solve this would be to allow things like |
@fenduru I'm afraid it is more difficult than that. Because the transclude slot parsing is done at compile time there is no chance for "dynamic" slots to be generated at link time (e.g. via I believe the only solution would be to go more down the lines of your library where all the transclusion slots are computed after compilation has finished. I discussed this with @IgorMinar and we felt that for the time being this was the best way forward, since it gives us performance and also compile time checking that required slots have been filled. |
@petebacondarwin I don't think requiring your slots to be present during compile time (i.e. part of your template) is unreasonable at all. On the other side though - the content that the user provided - we (directive authors) have no control over how the consumer puts the content there and would have to document that they cannot use ng-transclude/ng-include/similar to insert the child elements. This limitation unfortunately means it won't be usable for my use case (reusable low level components) since it would prevent composition, which is kind of the point of using transclusion. |
I am going to merge this in as it is. The ability to specify content for zones dynamically would use the same API so it is feasible it could be added in the future without a BC. |
Wooooohooooo! |
@petebacondarwin you're right it wouldn't be a breaking change. Do you know anything about transclusion in angular 2? Are the use cases I brought up being considered? |
I just managed to take a proper look (:flushed:). |
Thanks @gkalpak |
In Ng2 we are using |
Thanks for the info @tbosch |
Hello! Really looking forward to when this feature will be in the release. I decided to try how it will work. And I noticed that it is not working properly, now. Content from both ng-tansclude slots dublicates, instead of every slot to render its own content. It can can be seen in the multi-slot transclusion Plunker example, at official documentation. |
@ilyes-garifullin - can you clarify what the problem is? The demos look like they are working for me. |
The issue in the plunker is that it is pointing to 1.5.beta1 and the multi-transclude code is not there. If you change angular version in the plunker from beta1 to https://code.angularjs.org/snapshot/angular.js then it works as expected. This should get fixed once beta2 is out |
Oh, I see, thank you! |
I'm totally new to AngularJS, however, I believe I found an issue when jQuery is being included along with Angular, I could not reproduce the error when Angular is running without jQuery.
You can check the following Plnkr: http://plnkr.co/edit/f9SN35IMPrmeyuPz0Pih?p=preview
If you remove the jQuery inclusion it all goes well. |
From my quick tests looks like a tiny regression was introduced with this PR, in 1.4 this used to work: <div ng-transclude="ng-transclude"></div> But in 1.5 it throws an error:
Maybe Funny div(ng-transclude) |
OK @josip, that sounds reasonable. |
… its key Some preprocessors such as Jade will automatically provide a value for an attribute rather than leave it empty. E.g. `<div ng-transclude="ng-transclude">`. In these situations we still want to use the default transclusion slot. Closes angular#12934
I was also struggling to make multi ngTrasclude to work, even with beta 2. The Plunkr already points to beta 2 (1.5.0-beta.2), and it produces: However if I put ngTransclude keys and rename html elements as in this Plunkr I made, it seems to work fine: transclude: {
'paneb': 'paneb',
'panet': '?panet',
'panef': '?panef'
} and: <pane>
<panet><a ng-href="{{link}}">{{title}}</a></panet>
<paneb><p>{{text}}</p></paneb>
</pane> So something is fishy about those dashes... |
@gogromat - there has been a bit of churn this weekend with the API for multi-slot transclude. We are releasing RC1 today, in which the API should be stabilized. The transclude map should be: {
slotName: 'camelCaseElementName'
} |
In this version of multiple transclusion you specify the transclusion slots in the
transclude
property of the DDO, where the keys are the node names of transclusion zones and the values are the names of the slots in the template:So the following HTML
will be compiled and rendered to
Replaces #12742 and #11736