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

WIP - DO NOT MERGE - feat($compile): multiple transclusion via slots #12742

Closed

Conversation

petebacondarwin
Copy link
Contributor

This is a really rough and ready spike of multiple transclusion via slots.

There are LOTS of issues, primarily around the way that transclude functions are created, wrapped and passed around often hidden inside closures with access to a variety of other variables.
It is also notable that the transclude function, that gets passed to the directive's controller and link functions, is incompatible with the transclude functions (even the bound ones) that are passed around internally, since the former allows the scope parameter to be missing.
For this to be implemented properly I fear we may need to completely refactor large parts of the way transclusion is passed around inside the compiler.

Also it doesn't currently work for element transclusion.

This is an alternative strategy to #11736

@petebacondarwin
Copy link
Contributor Author

@kara and @IgorMinar do you want to take a quick look?

@petebacondarwin petebacondarwin added this to the 1.5.x - migration-facilitation milestone Sep 2, 2015
@petebacondarwin petebacondarwin force-pushed the multi-transclude branch 3 times, most recently from 47fe7b5 to 69308bb Compare September 3, 2015 07:18
@jbedard
Copy link
Contributor

jbedard commented Sep 3, 2015

Should this be accessible on the public $transclude function for cases where ng-transclude is not used? $transclude.slots.name maybe? I feel like this $compile feature should not be created only for ng-transclude...

@petebacondarwin
Copy link
Contributor Author

I agree. At this stage it is available on the transclude function via the convoluted:

$transclude.$$boundTransclude.$$slots[name]

See https://github.com/angular/angular.js/pull/12742/files#diff-348c2f3781ed66a24894c2046a52c628R7114

But it should be exposed more cleanly

$transclude = $transclude.$$boundTransclude.$$slots[$attrs.ngTransclude];
if (!$transclude) return;

$transclude(undefined, function(clone) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the first argument undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see your comments in the PR description.

@IgorMinar
Copy link
Contributor

This actually looks pretty reasonable. It would be great if we could complain about any slots defined in the transcluded template (light dom) but not in the transcluding template (shadow dom).

I don't think we need to support element transclusion. I don't think that it makes to mix that with slots.

Could we get away with not supporting imperative access to the transclusion function (via directive controller or link fn)?

@petebacondarwin
Copy link
Contributor Author

@IgorMinar can you clarify what you mean by this?

Could we get away with not supporting imperative access to the transclusion function (via directive controller or link fn)?

@fenduru
Copy link

fenduru commented Sep 9, 2015

I am a huge fan of the concept here, but I would like to voice my concern about the API. I work on a project where we build reusable components for other teams at my company. A typical component might look something like this:

<my-component>
  <my-component-header>Header</my-component-header>
  <my-component-content>Content</my-component-content>
  <my-component-footer>Footer</my-component-footer>
</my-component>

The nice thing about this is that during an initial implementation, myComponent might not even use transclusion, and this is an implementation detail completely hidden from my consumers. Then when new features are added in that require transclusion, this does not force me to make a breaking change to my API.

TL;DR please don't make me (reusable component author) export implementation detail as part of my API

FWIW I accomplished the API that I've described here by using $$TLB + transclude: element to re-define the child components, require the known parent, and transclude itself into the correct outlet. Usage looks like this (actual usage is easier because I've written some helpers):

module.directive('parentComponent', function(multiTranscludeController) {
  return {

    // Must pass the parent controller so that the slot registers with the correct controller even when there are multiple levels of nesting
    // Default slot is needed so that the children get linked
    template: '<transclude-slot name="default" parent="ctrl"></transclude-slot>' +
                    '<transclude-slot name="childGoesHere" parent="ctrl"></transclude-slot>',

    // This controller contains functions used to register both slots and children with the parent
    controller: multiTranscludeController,
    controllerAs: 'ctrl'
  }
})

.directive('childComponent', function() {
  // normal directive definition, do whatever you want here (including your own transclusion)
})

.directive('childComponent', function() {
  return {
    transclude: 'element',
    $$tlb: true,
    priority: 500,
    requre: '^^parentComponent',
    link: function() {
      // Register with the parent controller
    }
  };
})

@petebacondarwin
Copy link
Contributor Author

@fenduru I hear that you are saying that you want to have control over the API surface that your directive exposes to its clients.

I was initially nervous about this since it means that each directive must document what gets transcluded, and that users of directives must understand each directive's use of transclusion. In the way that is implemented in this PR users of all directives would just need to learn one rule which is about ng-slot. That being said, I am coming round to the idea of custom slot identifiers. Apart from anything else it is not clear in any case in a directive what slot names are required.

What do you think of the possible implementation described below:

@petebacondarwin
Copy link
Contributor Author

Define the transclusion slots via the transclude property on the directive definition object. So instead of only true or element you can provide a hash object:

{
  ...
  tranclude: {
    elementName1: 'transcludeSlot1',
    elementName2: '?optionalTranscludeSlot2',
    ...
  },
  ...
}

The keys match elements of the transcluded HTML that should be extracted into a slot.
The values match the names of the transclude slots, with a prefix of ? indicating that the slot is optional.

This would allow the directive designer to provide their own API for transclusion slots and also for the compiler to indicate an error if non-optional slots are not provided by the directive user.

So the usage would be:

<some-directive>
  <element-name1>... required element with content to be transcluded ...</element-name1>
  <element-name2>... this element is optional ...</element-name2>
</some-directive>

Internally the directive template would continue to use ng-transclude="slotName".

@fenduru
Copy link

fenduru commented Sep 11, 2015

@petebacondarwin That approach basically sounds like syntactic sugar for something like:

{
  transclude: true,
  link: function(scope, elem, attrs, ctrl, transclude) {
    transclude(function(clone) {
      clone.find('elementName1').appendTo(elem.find('transcludeSlot1'));
      clone.find('elementName2').appendTo(elem.find('transcludeSlot2'));
    });
  }
}

This approach is very naive IMO. It breaks down very quickly when you introduce things that have their own transclusion. For instance, what happens when you put ngIf on the optional elementName1? I would expect it to end up in the transclusion slot when the condition turns true, but this would not happen. Another case is when using ngInclude which is pretty much the only way to do recursive directives until #12078 lands.

I've spent a lot of time working on this class of problems, and I'm fairly confident that the only point where it is safe to handle this transclusion behavior is in the child elements link function. This is the only point where we know for sure that the child exists, and what it is inside of.

@petebacondarwin
Copy link
Contributor Author

What I suggested is not syntactic sugar for what you suggested. The implementation would be similar to what is in this PR.

@IgorMinar
Copy link
Contributor

@petebacondarwin I like the idea of specifying the map in DDO, would that help to solve the issue of accessing the transclusion functions for individual slots from within the linking function?

@petebacondarwin
Copy link
Contributor Author

I will create an alternative PR for this.

@petebacondarwin petebacondarwin self-assigned this Sep 14, 2015
@petebacondarwin
Copy link
Contributor Author

Even though this is not the final solution, I rebased it due to lazy compilation.

petebacondarwin added a commit that referenced this pull request Oct 12, 2015
Now you can efficiently split up and transclude content into specified
places in a component's template.

```html
<pane>
  <pane-title>Some content for slot A</pane-title>
  <pane-content>Some content for slot A</pane-content>
</component>
```

```js
mod.directive('pane', function() {
  return {
    restrict: 'E',
    transclude: { paneTitle: '?titleSlot', paneContent: 'contentSlot' },
    template:
    '<div class="pane">' +
      '<h1 ng-transclude="titleSlot"></h1>' +
      '<div ng-transclude="contentSlot"></div>' +
    '</div>' +
  };
});
```

Closes #4357
Closes #12742
Closes #11736
Closes #12934
@petebacondarwin petebacondarwin deleted the multi-transclude branch November 24, 2016 09:15
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.

5 participants