feat(portal): support context in TemplatePortal #6408

shlomiassaf · 2017-08-10T21:23:48Z

googlebot · 2017-08-10T21:23:51Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-08-10T21:40:03Z

CLAs look good, thanks!

jelbourn

Overall good change, the behavior is much more consistent between components and templates. Just some style/API comments

jelbourn · 2017-08-11T16:26:57Z

src/cdk/portal/portal.ts

-  attach(host: PortalHost, locals?: Map<string, any>): Map<string, any> {
-    this.locals = locals == null ? new Map<string, any>() : locals;
+  attach(host: PortalHost, context?: C): C {
+    this.context = context!;


Why use the ! here? The class property is marked as | undefined so it shouldn't be necessary

jelbourn · 2017-08-11T16:33:23Z

src/lib/dialog/dialog.ts

@@ -228,7 +228,9 @@ export class MdDialog {
    }

    if (componentOrTemplateRef instanceof TemplateRef) {
-      dialogContainer.attachTemplatePortal(new TemplatePortal(componentOrTemplateRef, null!));
+      dialogContainer.attachTemplatePortal(
+        new TemplatePortal<T>(componentOrTemplateRef, null!,


Doesn't null! resolve to never? Seems like this needs a real ViewContainerRef

This was the logic before I made the change so I left it as is.

I think if a change is required here it should be handled in a different PR not to mixup things.

It does work...

jelbourn · 2017-08-11T16:33:51Z

src/lib/snack-bar/snack-bar-container.ts

@@ -110,7 +111,8 @@ export class MdSnackBarContainer extends BasePortalHost implements OnDestroy {
  }

  /** Attach a template portal as content to this snack bar container. */
-  attachTemplatePortal(): Map<string, any> {
+  /* tslint:disable-next-line */


What's the lint problem here?

'C' is declared but never used.

Ah, you should be able to do

attachTemplatePortal(): EmbeddedViewRef<any> { ... }

@jelbourn
Yes, we can do that but this will not be inline with other attachTemplatePortal calls that does accept a type parameter. As well as the attachComponentPortal<T> above it.

This is not the only reason, main reason is that right now we need to add the tslint disable but once we move to TypeScript 2.4 it will be easy to migrate it to:

attachTemplatePortal<C = any>(): EmbeddedViewRef<C> { throw Error('Not yet implemented'); }

This way we allow setting the type parameter for those who want.
Later we will support those who "doesnt" want as well :)

And of course, once the function is actually implemented it will require a portal as a parameter so it will use the type param C

Using any here right now is the most technically correct thing, though (until the function is actually implemented). The linter is catching a real issue in that the function genuinely isn't generic.

No problem, will update it

jelbourn · 2017-08-11T16:35:22Z

src/cdk/portal/portal.spec.ts

@@ -299,15 +299,15 @@ describe('Portals', () => {
      fixture.detectChanges();

      // Attach the TemplatePortal.
-      testAppComponent.portalWithBinding.attach(host);
+      testAppComponent.portalWithBinding.attach(host, { $implicit: { status: 'fresh' } });


Omit spaces in braces

{$implicit: {status: 'fresh'}}

jelbourn · 2017-08-11T16:40:57Z

src/cdk/portal/portal.ts


-  constructor(template: TemplateRef<any>, viewContainerRef: ViewContainerRef) {
+  constructor(template: TemplateRef<any>, viewContainerRef: ViewContainerRef, context?: C) {


It's somewhat strange that the context can be set either in the constructor or in attach. Is this meant to be a default context when nothing is passed to attach?

This is required to support the abstraction in dialog.ts

If you follow the trail, the attach method is never called and BasePortalHost (PortalHostDirective) handles it from the top via attachTemplatePortal().

Now, adding a context as a param in the method attachTemplatePortal is a mess since it involves changing a lot of classes that extend BasePortal, for example MdDialogContainer

Additionally, changing the signature of attachTemplatePortal to accept a context will make it different from the signature attachComponentPortal...

So yes, there are 2 ways to set the context, one can override the other.

We can remove it from attach, the only effect here is that an extra line of code is required to set the context when call attach, just before one will need to do templatePortal.context = context

I see what you mean. I think it's okay to include it in the signature, then, so long as the JsDoc makes it clear that this updates the context property of the TemplatePortal

@jelbourn I'v added a JsDoc comment to the attach method acoordignly.

I didn't find an example of how to comment a constructor signature, and if its like commenting a method, so I left it as is.

FYI: Both template and viewContainerRef constructor parameters do the same thing (set a property on the instance) and are not commented so I don't think we should go into it. It is self explaining.

jelbourn · 2017-08-11T16:41:29Z

src/cdk/portal/portal.ts

    super();
    this.templateRef = template;
    this.viewContainerRef = viewContainerRef;
+    if (context) {
+     this.context = context;
+    }


You should just be able to put public context?: C in the constructor and omit this assignment

I just tried keeping the code style the same.

The other 2 parameters (template and viewContainerRef ) are also public but they are manually assigned without using TypeScript's constructor parameter modifiers....

So I just followed that.

I think that if we apply your suggestion it should be for all of the parameters of just leave it as is...

Ah, I think that's just from this being one of the first things I wrote in the library and not being as familiar with TypeScript back then.

jelbourn · 2017-08-11T16:41:53Z

src/cdk/portal/portal.ts

-  attach(host: PortalHost, locals?: Map<string, any>): Map<string, any> {
-    this.locals = locals == null ? new Map<string, any>() : locals;
+  attach(host: PortalHost, context?: C): C {
+    this.context = context!;


Won't this always override the context given in the constructor, even if no context is passed here? You should be able to do something like
attach(host, PortalHost, context: C = this.context)

Indeed.

I think this.context = context || this.context; is more readable but I used your approach

It requires a slight modification to be friends with typescript:

attach(host: PortalHost, context: C | undefined = this.context): C {

jelbourn · 2017-08-11T16:45:19Z

src/cdk/portal/portal.spec.ts

@@ -299,15 +299,15 @@ describe('Portals', () => {
      fixture.detectChanges();

      // Attach the TemplatePortal.
-      testAppComponent.portalWithBinding.attach(host);
+      testAppComponent.portalWithBinding.attach(host, { $implicit: { status: 'fresh' } });


There should be:

One test where no context is given

One test where context is given in the constructor

One test where context is given in attach

One test where context is given in both the constructor and in attach

Actually the tests are limited since there are no are no "low-level" tests for TemplatePortal with a PortalHostDirective (there are some for ComponentPortal)

I'v added 2 tests:

should load a template into the portal
Check basic projection of programatically created TemplatePortal

should project template context bindings in the portal
Check project of programatically created TemplatePortal with 4 internal expects

No context

Context through attach() method

Context through constructor

Context through constructor and through attach() method - the latter must take precedence.

jelbourn · 2017-08-11T16:45:59Z

src/lib/dialog/dialog.spec.ts

+    templateRefFixture.componentInstance.localValue = 'Bees';
+    templateRefFixture.detectChanges();
+
+    const data = { value: 'Knees' };


Omit spaces inside braces

refactor(portal): align TemplatePortal with ComponentPortal BREAKING CHANGE: `TemplatePortal` now requires a generic type (C), method `attach` return type C and property `locals` was removed method `attachTemplatePortal` in `BasePortalHost` now requires a generic type (C) and returns EmbeddedViewRef<C> (also applies to extending types `DomPortalHost`, `PortalHostDirective` and `MdDialogContainer`)

…mplate context

jelbourn

LGTM

shlomiassaf · 2017-08-15T17:01:46Z

@jelbourn Why does it fail?

jelbourn · 2017-08-15T18:51:02Z

e2e tests are just timing out, which happens sometimes

angular-automatic-lock-bot · 2019-09-06T22:02:39Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

shlomiassaf requested review from andrewseguin, crisbeto, jelbourn, josephperrott and kara as code owners August 10, 2017 21:23

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Aug 10, 2017

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 10, 2017

jelbourn reviewed Aug 11, 2017

View reviewed changes

jelbourn added the pr: needs review label Aug 11, 2017

shlomiassaf added 4 commits August 13, 2017 18:18

feat(dialog): pass user defined data and MdDialogRef instance to te…

aa633c4

…mplate context

chore: fix linting, tests and peer review adjustments

7c1bb88

chore: updates following peer review comments

d92f913

jelbourn approved these changes Aug 15, 2017

View reviewed changes

kara removed their request for review August 16, 2017 01:04

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Aug 16, 2017

kara merged commit 90a6ac9 into angular:master Aug 21, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(portal): support context in TemplatePortal #6408

feat(portal): support context in TemplatePortal #6408

shlomiassaf commented Aug 10, 2017

googlebot commented Aug 10, 2017

googlebot commented Aug 10, 2017

jelbourn left a comment •

edited

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn Aug 14, 2017

shlomiassaf Aug 14, 2017 •

edited

jelbourn Aug 14, 2017

shlomiassaf Aug 15, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017 •

edited

jelbourn Aug 14, 2017

shlomiassaf Aug 15, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017 •

edited

jelbourn Aug 14, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn Aug 11, 2017

shlomiassaf Aug 13, 2017

jelbourn left a comment

shlomiassaf commented Aug 15, 2017

jelbourn commented Aug 15, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019


		constructor(template: TemplateRef<any>, viewContainerRef: ViewContainerRef) {
		constructor(template: TemplateRef<any>, viewContainerRef: ViewContainerRef, context?: C) {

feat(portal): support context in TemplatePortal #6408

feat(portal): support context in TemplatePortal #6408

Conversation

shlomiassaf commented Aug 10, 2017

googlebot commented Aug 10, 2017

googlebot commented Aug 10, 2017

jelbourn left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomiassaf Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomiassaf Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomiassaf Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

shlomiassaf commented Aug 15, 2017

jelbourn commented Aug 15, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

jelbourn left a comment •

edited

shlomiassaf Aug 14, 2017 •

edited

shlomiassaf Aug 13, 2017 •

edited

shlomiassaf Aug 13, 2017 •

edited