feat(dialog): add ability to open dialog with templateRef #2853

fxck · 2017-01-28T21:57:14Z

I split #2851, used TemplateRef instead of TemplatePortal, and committed only the part that adds openFromTemplateRef().

Added tests for backdropClicked, escapePressed(I explained the need for these in the previous PR) and a test for creating a dialog using templateRef. Not sure which other tests would be needed, I think the rest is identical to opening a dialog with open().

Then there's a little "problem" that components inside the <template> do not have access to MdDialogRef, so dialogRef.close() and md-dialog-close obviously do not work, but if you are opening a dialog using TemplateRef, you likely do not need those anyway.

@jelbourn @crisbeto @devversion

fxck · 2017-01-29T10:33:01Z

Fixed tests and added separated demo dialog components because of the fact that dialog opened with TemplateRef does not have MdDialogRef.

jelbourn · 2017-01-30T17:08:25Z

src/lib/dialog/dialog-container.ts

+    return attachResult;
+  }
+
+  attachTemplateRef<T>(templateRef: TemplateRef<T>) {


The TemplateRef should be instantiated in dialog.ts and we should just call attach on the dialog container (from BasePortalHost) that will delegate to the appropriate method.

I'm not sure I understand this correctly, but to be able to create a portal, you need ViewContainerRef, which is not accessible inside dialog.ts. In my previous PR I had MdDialog.openFromPortal(), but I think it a little too tedious for user to have to create a portal rather than to be just able to pass in TemplateRef. But I don't really mind either.

I see, I could make ViewTemplateRef available to dialog.ts.

What about types thought, wouldn't you lose them by just calling attach instead of attachComponentPortal/attachTemplatePortal?

Also if I just use attach, typescript will complain about not properly implementing attachTemplatePortal and attachComponentPortal.. what should I do please, @jelbourn ?

jelbourn · 2017-01-30T17:11:54Z

src/lib/dialog/dialog-ref.ts

+  backdropClicked: Observable<void>;
+
+  /** Subject for notifying the user that esc key way pressed */
+  escapePressed = new Subject<void>();


FYI: the Subject shouldn't ever be a public property, only the Observable

asObservable() then?

The problem is that I need to next it from inside dialog-container.ts, if I make it private I won't be able to access it.

jelbourn · 2017-01-30T17:13:40Z

src/lib/dialog/dialog-ref.ts

@@ -14,10 +14,18 @@ export class MdDialogRef<T> {
  /** The instance of component opened into the dialog. */
  componentInstance: T;

+  /** Expose overlay backdrop click event */
+  backdropClicked: Observable<void>;


Should be backdropClick (to match OverlayRef).

I don't think there needs to be an API for escapePressed. Users should be able to add their own keydown or keypress handler to their dialog content.

That's what I thought first as well, but then there is afterClosed instead of afterClose, but I was not sure.

As for escape, there is handleEscapeKey() on dialog-container.ts already, so I think it's fair to expose that.

jelbourn · 2017-01-30T17:17:37Z

src/lib/dialog/dialog.ts

+   * @param config Extra configuration options.
+   * @returns Reference to the newly-opened dialog.
+   */
+  openFromTemplateRef<T>(templateRef: TemplateRef<T>, config?: MdDialogConfig): MdDialogRef<T> {


Just openFromTemplate

jelbourn · 2017-01-30T17:51:05Z

src/lib/dialog/dialog.ts

+  openFromTemplateRef<T>(templateRef: TemplateRef<T>, config?: MdDialogConfig): MdDialogRef<T> {
+    config = _applyConfigDefaults(config);
+
+    let overlayRef = this._createOverlay(config);


Rather than duplicating the control flow, open and openFromTemplate should just instantiate the appropriate Portal and then pass that through. One of the benefits of portals is that the host doesn't have to care which kind it is.

Similar problem as with #2853 (comment) I don't think I can do that unless I have ViewContainerRef, which I don't unless I require the user to pass me TemplatePortal.. which I think would be quite redundant anyway, since dialog-container's ViewContainerRef is that I really need anyway, right?

Not sure I get the issue here. Isn't it down to doing a ref instanceof TemplateRef check and then taking the proper route under the hood?

jelbourn · 2017-01-30T17:55:19Z

src/lib/dialog/dialog.ts

+   * @param config Extra configuration options.
+   * @returns Reference to the newly-opened dialog.
+   */
+  openFromTemplateRef<T>(templateRef: TemplateRef<T>, config?: MdDialogConfig): MdDialogRef<T> {


I think this method should lose the generic and return MdDialogRef<void>. since the generic refers to the type of componentInstance.

Yeah, I was not sure about what to do with that(#2851 read 2.).

crisbeto · 2017-01-30T19:05:02Z

src/lib/dialog/dialog-ref.ts

+  backdropClicked: Observable<void>;
+
+  /** Subject for notifying the user that esc key way pressed */
+  escapePressed = new Subject<void>();


I'm not sure that we need observables for escapePressed, backdropClicked and afterClosed since the API is already becoming pretty dense. I'd rather we kept everything to afterClosed and emitted an object that looks something like { result: <user-provided value>, source: 'backdrop' | 'keypress' | 'api' }.

That wouldn't trigger if escape/backdrop click didn't actually close the dialog. Which in result wouldn't allow for creating a stateless dialog component(which is honestly a must have in redux and ngrx/store world). Or afterClosed could be modified to trigger and not close the dialog when disableClose is true, but then the name wouldn't make much sense.

EDIT: I see what you mean, but why would the user want to know that the backdrop was clicked and it didn't result in a close?

You could differentiate it based on the source. My worry is that we're ending up with 5-6 different dialog-related observables. If we added these, we'd have 3 just for the close method.

What could I differentiate? The problem is that afterClosed() triggers only when close is called, which actually closes the dialog . I need to be able to catch any close attempt while disableClose is true and let user know about it, so he can modify the state himself. Would you be happier is backdrop and escape were merged into one observable, say closeAttempt?

My question is why does the user need to know that an attempt to close it was made?

Check #2855

Imagine you keep state of your modal in a global store, like redux or @ngrx/store, so you create a component that looks like this.

<my-dialog [open]="modalState$ | async"> <button (click)="doSomethingThatChangesModalState$toFalse()">close</button> </my-dialog>

Now you are in full control of the state, when you change modalState$, your modal closes, BUT when you click on backdrop or press escape, your modal closes while your modalState$ is still true and your store is suddenly out of sync. You could catch up by changing modalState$ to false on afterClose(), but that could for example be called only after the animation finishes. It simply smells, it's the other way around than it should be. So you want to disable the default closing behaviour of backdropClick and escape, so you do disableClose: true, but then you have to manually set up listeners for both, which will be big pain in the ass(or it might not even be possible at all).

So ideally you want to do

<my-dialog [open]="modalState$ | async" (close)="doSomethingThatChangesModalState$toFalse()"> <button (click)="doSomethingThatChangesModalState$toFalse()">close</button> </my-dialog>

where close event happens on either backdropClick or escape press https://github.com/fxck/material2/blob/662703b63831797c9969dda4dd03d69d794aa1f4/src/lib/dialog/dialog-element.ts#L54-L61

It's what's called stateless components and what you want to do when using redux or store. There are other components where this would make sense, for example keeping sidenav state inside ngrx/store is currently a bit of a pain in the ass as well.

Alright, it makes sense. I still think that they should be combined into something along the lines of userGeneratedClose.

crisbeto · 2017-01-30T19:08:31Z

src/lib/dialog/dialog.ts

+  openFromTemplateRef<T>(templateRef: TemplateRef<T>, config?: MdDialogConfig): MdDialogRef<T> {
+    config = _applyConfigDefaults(config);
+
+    let overlayRef = this._createOverlay(config);


Not sure I get the issue here. Isn't it down to doing a ref instanceof TemplateRef check and then taking the proper route under the hood?

crisbeto · 2017-01-30T19:09:09Z

src/lib/dialog/dialog.ts

+    let dialogRef = this._attachDialogContent(
+      dialogContainer,
+      overlayRef,
+      undefined,


Probably better to pass in null here instead. That way we know that it was skipped on purpose.

https://medium.com/@basarat/null-vs-undefined-in-typescript-land-dc0c7a5f240a#.ug5uqgtus but sure, I guess rest of the lib is not following this anyway

crisbeto · 2017-01-30T19:10:22Z

src/lib/dialog/dialog.ts

@@ -143,10 +174,16 @@ export class MdDialog {
    let userInjector = config && config.viewContainerRef && config.viewContainerRef.injector;
    let dialogInjector = new DialogInjector(dialogRef, userInjector || this._injector);

-    let contentPortal = new ComponentPortal(component, null, dialogInjector);
+    if (component) {


I think that the logic that is specific to TemplateRef and ComponentType should be split into separate private methods.

crisbeto · 2017-01-30T19:12:07Z

src/lib/dialog/dialog.spec.ts

+
+    let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
+
+    let clicked = false;


Instead of keeping a flag like this, you should use a spy:

let clickedSpy = jasmine.createSpy('clicked callback'); dialogRef.backdropClicked.subscribe(clickedSpy); expect(clickedSpy).toHaveBeenCalled();

crisbeto · 2017-01-30T19:12:36Z

src/lib/dialog/dialog.spec.ts

+    let dialogContainer: MdDialogContainer =
+        viewContainerFixture.debugElement.query(By.directive(MdDialogContainer)).componentInstance;
+
+    let pressed = false;


This should use a spy as well. See https://github.com/angular/material2/pull/2853/files#r98516864

fxck · 2017-01-30T21:00:30Z

Addressed couple of comments:

use spy(feat(dialog): add ability to open dialog with templateRef #2853 (comment), feat(dialog): add ability to open dialog with templateRef #2853 (comment))
split logic specific to template/component to private methods feat(dialog): add ability to open dialog with templateRef #2853 (comment)
use null instead of undefined feat(dialog): add ability to open dialog with templateRef #2853 (comment)
remove generics feat(dialog): add ability to open dialog with templateRef #2853 (comment)
create TemplatePortal in dialog.ts feat(dialog): add ability to open dialog with templateRef #2853 (comment)
rename method to openFromTemplate feat(dialog): add ability to open dialog with templateRef #2853 (comment)
rename clicked / pressed to click / press feat(dialog): add ability to open dialog with templateRef #2853 (comment)

Didn't address these:

remove backdropClicked / escapePressed, reasons explained in feat(dialog): add ability to open dialog with templateRef #2853 (comment)
- what I could do here is to merge these two into one
don't expose subject feat(dialog): add ability to open dialog with templateRef #2853 (comment)
- I cannot make it private, since I need to call it from inside dialog-container.ts.. not sure what to do here, create a public function? that will still make it available to consumers
use attach instead of attachComponentPortal and attachTemplatePortal feat(dialog): add ability to open dialog with templateRef #2853 (comment)
- not sure what to do here, by using attach I would lose types and typescript will complain, because I won't be properly implementing BasePortalHost

@jelbourn @crisbeto

fxck · 2017-01-30T21:17:57Z

src/lib/dialog/dialog-container.ts

    super();
+
+    this.containerRef = this._viewContainerRef;


had to do this, when I did just

constructor(viewContainerRef: ViewContainerRef, private _ngZone: NgZone) {}

this https://github.com/fxck/material2/blob/9a0a28be6b551302363577462865fac760ad09a3/src/lib/dialog/dialog.ts#L212 was complaining that viewContainerRef doesn't exist

crisbeto · 2017-01-31T08:20:55Z

@fxck regarding your comment about not being able to expose the escapePressed subject, I'm not sure that the DialogRef should be the one that knows about what DOM events are happening in the DialogContainer. In #2825 I changed the relationship from it being container->ref to ref-container, which means that you could keep the subject in the container and do a dialogRef.containerInstance.escapePressed.subscribe(). Note that the containerInstance is private in #2825, but that can be refactored if necessary.

fxck · 2017-01-31T08:57:58Z

I could probably base my PR on yours if you want. I actually was wondering about another thing regarding your PR, it currently doesn't take in account from what button was the dialog opened(like material1 does), but once that's there, I'm not sure how's that gonna work with element like this #2855

crisbeto · 2017-01-31T09:12:19Z

There isn't anything in the spec that says that it's supposed to do that, and I couldn't find anybody that remembered why it was done that way in M1. The only reference to dialog animations in the spec is this upon which I based my PR. After looking through different implementations (Google Docs, Google Keep, Inbox, Android) it's pretty inconsistent.

fxck · 2017-01-31T12:00:49Z

I think it might have something to do with one of these https://storage.googleapis.com/material-design/publish/material_v_10/assets/0B14F_FSUCc01dFhwekx5R3BsNGM/Radial_04_RadialArc_v5-remapped.mp4 .. it's gotta be said it arguably looks pretty good in m1..

Anyway, how should I proceed with this PR then? I'll do anything you want/need as long as it allows me to create that stateless dialog component/directive. I would really love to get this in, it's been bothering me for a long long time.

crisbeto · 2017-01-31T12:06:55Z

I would say to go with the assumption from #2825 and if we decide to go the M1 route, I think that it's not too complicated to refactor.

fxck · 2017-01-31T12:32:33Z

I guess I'll wait until that gets merged then.

angular-automatic-lock-bot · 2019-09-06T09:01:46Z

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.}

add ability to open dialog with templateRef

ad9617f

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 28, 2017

Ales Rechtorik added 3 commits January 29, 2017 11:10

fix lint

14ef81c

fix dialog templateRef test

bd19bd3

separate component for content elements dialog demo with templateRef

dbb1676

add missing closing tag

1803505

fxck mentioned this pull request Jan 29, 2017

feat(dialog): add md-dialog component #2855

Closed

jelbourn requested a review from crisbeto January 30, 2017 17:00

jelbourn reviewed Jan 30, 2017

View reviewed changes

crisbeto requested changes Jan 30, 2017

View reviewed changes

address comments

9a0a28b

fxck commented Jan 30, 2017

View reviewed changes

fxck mentioned this pull request Jan 30, 2017

[Investigation] Improve Reactive Forms Experience ngrx/core#12

Open

fxck mentioned this pull request Feb 4, 2017

feat(dialog): support open with TemplateRef #2910

Merged

fxck mentioned this pull request Feb 23, 2017

expose dialog close attempts #3251

Closed

fxck closed this Feb 27, 2017

fxck mentioned this pull request Mar 6, 2017

feat(dialog): expose close attempts #3460

Closed

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


		let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;

		let clicked = false;

feat(dialog): add ability to open dialog with templateRef #2853

feat(dialog): add ability to open dialog with templateRef #2853

Conversation

fxck commented Jan 28, 2017 • edited Loading

fxck commented Jan 29, 2017 • edited Loading

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

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

fxck Jan 30, 2017 • edited Loading

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

crisbeto Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxck Jan 30, 2017 • edited Loading

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

fxck commented Jan 30, 2017

fxck Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

crisbeto commented Jan 31, 2017 • edited Loading

fxck commented Jan 31, 2017

crisbeto commented Jan 31, 2017

fxck commented Jan 31, 2017 • edited Loading

crisbeto commented Jan 31, 2017

fxck commented Jan 31, 2017

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

fxck commented Jan 28, 2017 •

edited

Loading

fxck commented Jan 29, 2017 •

edited

Loading

fxck Jan 30, 2017 •

edited

Loading

crisbeto Jan 30, 2017 •

edited

Loading

crisbeto Jan 30, 2017 •

edited

Loading

fxck Jan 30, 2017 •

edited

Loading

fxck Jan 30, 2017 •

edited

Loading

crisbeto commented Jan 31, 2017 •

edited

Loading

fxck commented Jan 31, 2017 •

edited

Loading