Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ad9617f
add ability to open dialog with templateRef
Jan 28, 2017
14ef81c
fix lint
Jan 29, 2017
bd19bd3
fix dialog templateRef test
Jan 29, 2017
dbb1676
separate component for content elements dialog demo with templateRef
Jan 29, 2017
1803505
add missing closing tag
Jan 29, 2017
9a0a28b
address comments
Jan 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/demo-app/demo-app-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ import {MaterialModule, OverlayContainer,
FullscreenOverlayContainer} from '@angular/material';
import {DEMO_APP_ROUTES} from './demo-app/routes';
import {ProgressBarDemo} from './progress-bar/progress-bar-demo';
import {JazzDialog, ContentElementDialog, DialogDemo, IFrameDialog} from './dialog/dialog-demo';
import {
JazzDialog,
JazzDialogTemplateRef,
ContentElementDialog,
ContentElementTemplateRefDialog,
DialogDemo,
IFrameDialog
} from './dialog/dialog-demo';
import {RippleDemo} from './ripple/ripple-demo';
import {IconDemo} from './icon/icon-demo';
import {GesturesDemo} from './gestures/gestures-demo';
Expand Down Expand Up @@ -64,7 +71,9 @@ import {InputContainerDemo} from './input/input-container-demo';
IconDemo,
InputContainerDemo,
JazzDialog,
JazzDialogTemplateRef,
ContentElementDialog,
ContentElementTemplateRefDialog,
IFrameDialog,
ListDemo,
LiveAnnouncerDemo,
Expand Down Expand Up @@ -100,7 +109,9 @@ import {InputContainerDemo} from './input/input-container-demo';
entryComponents: [
DemoApp,
JazzDialog,
JazzDialogTemplateRef,
ContentElementDialog,
ContentElementTemplateRefDialog,
IFrameDialog,
RotiniPanel,
ScienceJoke,
Expand Down
21 changes: 19 additions & 2 deletions src/demo-app/dialog/dialog-demo.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
<h1>Dialog demo</h1>

<button md-raised-button color="primary" (click)="openJazz()" [disabled]="dialogRef">Open dialog</button>
<button md-raised-button color="accent" (click)="openContentElement()">Open dialog with content elements</button>
<div class="demo-button-group">
<button md-raised-button color="primary" (click)="openJazz()" [disabled]="dialogRef">Open dialog</button>
<button md-raised-button color="accent" (click)="openContentElement()">Open dialog with content elements</button>
</div>

<div class="demo-button-group">
<button md-raised-button color="primary" (click)="openJazzUsingTemplateRef()" [disabled]="dialogTemplateRef">Open dialog using TemplateRef</button>
<button md-raised-button color="accent" (click)="openContentElementUsingTemplateRef()">Open dialog using TemplateRef with content elements</button>
</div>


<md-card class="demo-dialog-card">
<md-card-content>
Expand Down Expand Up @@ -51,3 +59,12 @@ <h2>Other options</h2>
</md-card>

<p>Last close result: {{lastCloseResult}}</p>

<template #jazzDialogRef>
<demo-jazz-dialog-template-ref (close)="closeJazzUsingTemplateRef($event)"></demo-jazz-dialog-template-ref>
</template>


<template #contentElementRef>
<demo-content-element-template-ref-dialog (close)="closeContentElementUsingTemplateRef()" [actionsAlignment]="actionsAlignment"></demo-content-element-template-ref-dialog>
</template>
4 changes: 4 additions & 0 deletions src/demo-app/dialog/dialog-demo.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
max-width: 350px;
margin: 20px 0;
}

.demo-button-group {
margin-bottom: 20px;
}
126 changes: 122 additions & 4 deletions src/demo-app/dialog/dialog-demo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import {Component, Inject} from '@angular/core';
import {
Component,
Inject,
Input,
Output,
ViewChild,
TemplateRef,
EventEmitter
} from '@angular/core';
import {DOCUMENT} from '@angular/platform-browser';
import {MdDialog, MdDialogRef, MdDialogConfig} from '@angular/material';

Expand All @@ -9,7 +17,15 @@ import {MdDialog, MdDialogRef, MdDialogConfig} from '@angular/material';
styleUrls: ['dialog-demo.css'],
})
export class DialogDemo {
@ViewChild('jazzDialogRef')
jazzDialogRef: TemplateRef<JazzDialogTemplateRef>;

@ViewChild('contentElementRef')
contentElementRef: TemplateRef<ContentElementDialog>;

dialogRef: MdDialogRef<JazzDialog>;
dialogTemplateRef: MdDialogRef<JazzDialogTemplateRef>;
dialogContentTemplateRef: MdDialogRef<ContentElementDialog>;
lastCloseResult: string;
actionsAlignment: string;
config: MdDialogConfig = {
Expand Down Expand Up @@ -41,16 +57,43 @@ export class DialogDemo {
openJazz() {
this.dialogRef = this.dialog.open(JazzDialog, this.config);

this.dialogRef.afterClosed().subscribe(result => {
this.dialogRef.afterClosed().first().subscribe(result => {
this.lastCloseResult = result;
this.dialogRef = null;
});
}

openJazzUsingTemplateRef() {
this.dialogTemplateRef = this.dialog.openFromTemplateRef(this.jazzDialogRef, this.config);

this.dialogTemplateRef.afterClosed().first().subscribe(() => {
this.dialogTemplateRef = null;
});
}

closeJazzUsingTemplateRef(result: string) {
this.lastCloseResult = result;

this.dialogTemplateRef.close();
}

openContentElement() {
let dialogRef = this.dialog.open(ContentElementDialog, this.config);
dialogRef.componentInstance.actionsAlignment = this.actionsAlignment;
}

openContentElementUsingTemplateRef() {
this.dialogContentTemplateRef = this.dialog.openFromTemplateRef(
this.contentElementRef,
this.config
);
}

closeContentElementUsingTemplateRef() {
if (this.dialogContentTemplateRef) {
this.dialogContentTemplateRef.close();
}
}
}


Expand All @@ -69,6 +112,24 @@ export class JazzDialog {
}


@Component({
selector: 'demo-jazz-dialog-template-ref',
template: `
<p>It's Jazz!</p>
<p><label>How much? <input #howMuch></label></p>
<p> {{ jazzMessage }} </p>
<button type="button" (click)="close.emit(howMuch.value)">Close dialog</button>`
})
export class JazzDialogTemplateRef {
jazzMessage = 'Jazzy jazz jazz';

@Output()
close = new EventEmitter<string>(false);

constructor() { }
}


@Component({
selector: 'demo-content-element-dialog',
styles: [
Expand Down Expand Up @@ -98,13 +159,12 @@ export class JazzDialog {
md-raised-button
color="primary"
md-dialog-close>Close</button>

<a
md-button
color="primary"
href="https://en.wikipedia.org/wiki/Neptune"
target="_blank">Read more on Wikipedia</a>

<button
md-button
color="secondary"
Expand All @@ -123,6 +183,64 @@ export class ContentElementDialog {
}
}


@Component({
selector: 'demo-content-element-template-ref-dialog',
styles: [
`img {
max-width: 100%;
}`
],
template: `
<h2 md-dialog-title>Neptune</h2>

<md-dialog-content>
<img src="https://upload.wikimedia.org/wikipedia/commons/5/56/Neptune_Full.jpg"/>

<p>
Neptune is the eighth and farthest known planet from the Sun in the Solar System. In the
Solar System, it is the fourth-largest planet by diameter, the third-most-massive planet,
and the densest giant planet. Neptune is 17 times the mass of Earth and is slightly more
massive than its near-twin Uranus, which is 15 times the mass of Earth and slightly larger
than Neptune. Neptune orbits the Sun once every 164.8 years at an average distance of 30.1
astronomical units (4.50×109 km). It is named after the Roman god of the sea and has the
astronomical symbol ♆, a stylised version of the god Neptune's trident.
</p>
</md-dialog-content>

<md-dialog-actions [attr.align]="actionsAlignment">
<button
md-raised-button
color="primary"
(click)="!!close.emit()">Close</button>
<a
md-button
color="primary"
href="https://en.wikipedia.org/wiki/Neptune"
target="_blank">Read more on Wikipedia</a>

<button
md-button
color="secondary"
(click)="showInStackedDialog()">
Show in Dialog</button>
</md-dialog-actions>
`
})
export class ContentElementTemplateRefDialog {
@Input()
actionsAlignment: string;

@Output()
close = new EventEmitter<void>();

constructor(public dialog: MdDialog) { }

showInStackedDialog() {
this.dialog.open(IFrameDialog);
}
}

@Component({
selector: 'demo-iframe-dialog',
styles: [
Expand Down
41 changes: 31 additions & 10 deletions src/lib/dialog/dialog-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import {
ViewEncapsulation,
NgZone,
OnDestroy,
ViewContainerRef,
TemplateRef,
} from '@angular/core';
import {BasePortalHost, ComponentPortal, PortalHostDirective, TemplatePortal} from '../core';
import {MdDialogConfig} from './dialog-config';
Expand Down Expand Up @@ -46,7 +48,7 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
/** Reference to the open dialog. */
dialogRef: MdDialogRef<any>;

constructor(private _ngZone: NgZone) {
constructor(private _viewContainerRef: ViewContainerRef, private _ngZone: NgZone) {
super();
}

Expand All @@ -61,20 +63,27 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {

let attachResult = this._portalHost.attachComponentPortal(portal);

// If were to attempt to focus immediately, then the content of the dialog would not yet be
// ready in instances where change detection has to run first. To deal with this, we simply
// wait for the microtask queue to be empty.
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
this._elementFocusedBeforeDialogWasOpened = document.activeElement;
this._focusTrap.focusFirstTabbableElement();
});
this._focusFirstTabbableElement();

return attachResult;
}

/** @docs-private */
attachTemplatePortal(portal: TemplatePortal): Map<string, any> {
throw Error('Not yet implemented');
if (this._portalHost.hasAttached()) {
throw new MdDialogContentAlreadyAttachedError();
}

let attachResult = this._portalHost.attachTemplatePortal(portal);

this._focusFirstTabbableElement();

return attachResult;
}

attachTemplateRef<T>(templateRef: TemplateRef<T>) {
Copy link
Member

@jelbourn jelbourn Jan 30, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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

const portal = new TemplatePortal(templateRef, this._viewContainerRef);

this.attachTemplatePortal(portal);
}

/**
Expand All @@ -85,6 +94,8 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
if (!this.dialogConfig.disableClose) {
this.dialogRef.close();
}

this.dialogRef.escapePressed.next();
}

ngOnDestroy() {
Expand All @@ -95,4 +106,14 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
(this._elementFocusedBeforeDialogWasOpened as HTMLElement).focus();
});
}

private _focusFirstTabbableElement() {
// If were to attempt to focus immediately, then the content of the dialog would not yet be
// ready in instances where change detection has to run first. To deal with this, we simply
// wait for the microtask queue to be empty.
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
this._elementFocusedBeforeDialogWasOpened = document.activeElement;
this._focusTrap.focusFirstTabbableElement();
});
}
}
10 changes: 9 additions & 1 deletion src/lib/dialog/dialog-ref.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Member

@jelbourn jelbourn Jan 30, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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.


/** Subject for notifying the user that esc key way pressed */
escapePressed = new Subject<void>();
Copy link
Member

@jelbourn jelbourn Jan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

asObservable() then?

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@crisbeto crisbeto Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@crisbeto crisbeto Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@fxck fxck Jan 30, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@crisbeto crisbeto Jan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fxck fxck Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@crisbeto crisbeto Jan 31, 2017

Choose a reason for hiding this comment

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

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


/** Subject for notifying the user that the dialog has finished closing. */
private _afterClosed: Subject<any> = new Subject();

constructor(private _overlayRef: OverlayRef) { }
constructor(private _overlayRef: OverlayRef) {
this.backdropClicked = this._overlayRef.backdropClick();
}

/**
* Close the dialog.
Expand Down
Loading