feat(dialog): inital framework for md-dialog #761
Conversation
googlebot
added
the
cla: yes
| import {Component, ComponentRef, ViewChild, AfterViewInit} from '@angular/core'; | ||
| import {BasePortalHost, ComponentPortal, TemplatePortal} from '../../core/portal/portal'; | ||
| import {PortalHostDirective} from '../../core/portal/portal-directives'; | ||
| import {PromiseCompleter} from '../../core/async/promise-completer'; |
There was a problem hiding this comment.
Yep, I just forgot to correct what WebStorm auto-imported. Fixed.
| @@ -0,0 +1 @@ | |||
| <template portalHost></template> | |||
There was a problem hiding this comment.
1 liner should probably be inlined.
There was a problem hiding this comment.
There will be more; this is just the first PR
|
LGTM aside from the imports that @iveysaur pointed out (which happens in a few files). |
| * @returns A promise resolving to the MdDialogRef that should be returned to the user. | ||
| */ | ||
| private _attachDialogContent<T>( | ||
| component: {new (): T}, |
There was a problem hiding this comment.
This type differs than the component declaration above.
You should abstract this to:
export interface GenericType<T> {
new (...args: any[]): T;
}
export type Type = GenericType<any>;I tested it in your PR and it works as expected. Having a reusable interface for this will make it way more readable and avoid questions with this "weird" syntax.
|
I think it could be useful to split this PR into two parts; one for the generic portal implentation and one for the dialog implementation that uses it. The reason for this split is simple to enable injection into portal objects as soon as possible without waiting for the dialog implementation which could take a while. |
jelbourn
force-pushed
the
dialog-concept
branch
4 times, most recently
from
9b551f5
to
e71b56b
Compare
|
@robertmesserle @hansl addressed comments |
| // If there was an attempted call to `attachComponentPortal` before this lifecycle stage, | ||
| // we actually perform the attachment now that the `@ViewChild` is resolved. | ||
| if (this._deferredAttachCompleter) { | ||
| this.attachComponentPortal(this._deferredAttachPortal).then(componentRef => { |
|
Outside of the assertion and good citizenry regarding promises, LGTM. |
|
|
||
| /** Runs the necessary detectChanges for a dialog to complete its opening. */ | ||
| function detectChangesForDialogOpen(fixture: ComponentFixture<ComponentWithChildViewContainer>) { | ||
| // TODO(jelbourn): figure out why the test zone is "stable" when where are still pending |
|
Comments addressed. Needs LGTM label so that caretaker knows to merge it. |
|
|
||
| this._deferredAttachPortal = null; | ||
| this._deferredAttachCompleter = null; | ||
| }, () => this._deferredAttachCompleter.reject()); |
|
LGTM. |
|
wahoo!!! 🎉 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
R: @hansl @kara @robertmesserle
CC: @iveysaur
ComponentPortalMdDialogservice that can open a dialog (and literally nothing else). Additional APIs will come in subsequent PRs. Dialog is also yet unstyled.The
setTimeouthack in the dialog tests is an unfortunate consequence of an issue with either zones or (more likely) the testing infrastructure built on top of it. I'm going to sit down with Misko when he gets back from vacation to get to the root of it.