Skip to content
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.

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

WIP: feature(dialog): add ability to open dialog with portal #2851

Closed
wants to merge 1 commit into from

Conversation

fxck
Copy link
Contributor

@fxck fxck commented Jan 28, 2017

I haven't really created this with hope of it being merged, but rather to get some feedback. There are some unused imports, couple of missing comments, no tests, I'll fix all of those if you tell me if this direction is fine.

  1. I tried to make opening just by using TemplateRef work, but I couldn't quite figure out which would be the proper viewContainerRef to use, there's still no easy way to get root component's viewContainerRef, right? I think it could probably use MdDialogContainer, viewContainerRef, so I could change it to that if you say it's a good idea(here's a branch with that https://github.com/fxck/material2/tree/dialog-templateref)

  2. MdDialog, MdDialogRef and various methods require generics of a component, should it still be required when using templateRef/portal? You don't need to expose componentInstance when using templateRef, you can simply use standard inputs.. how should this be reflected?

  3. I needed to expose backdropClick event(to be able to make truly stateless components, will explain later), is the way I'e done it ok(apart from $ suffix which I think you don't really use)

  4. I wanted to reuse _attachDialogContent, tried couple of things, but the only viable way ended up being adding ability to pass in either component or portal(https://github.com/fxck/material2/blob/15313f94ae75b6464054ada0bbad353eb1c9a0f8/src/lib/dialog/dialog.ts#L168-L169), I don't like it, but since it's a private, internal method, it shouldn't really be that bad, right?

  5. I've added md-dialog component, which is a component with api following that of https://developer.mozilla.org/en/docs/Web/HTML/Element/dialog I know you are not fan of that, so it just temporar to test my usecase for opening dialog with templateRef/portal, and I encountered one problem, and that is

Expression has changed after it was checked. Previous value: 'CD_INIT_VALUE'. Current value 'dialog'

it happens when I try to open the dialog inside @Input setter, I think let overlayRef = this._createOverlay(config); is what's causing it.. it was fixed by using setTimeout, which would be "fine" if it was internal(I assume some zone function instead of setTimeoutwould work as well), but you can't expect that from the consumer.. so I could use some help fixing it


anyway the idea behind the md-dialog component, should you decode it's something you'd want in the repo, is that it's completely stateless by default, which means the consumer is in full control of the state, backdrop / esc key should not close it, they should expose and event that close attempt happened and let the user decide what to do with the state instead.. it's something you totally need when using state manager like https://github.com/ngrx/store I think this behaviour should be by default, but I think it could use another input with config(MdDialogConfig), where one could turn disableClose back on

@jelbourn @crisbeto @devversion maybe?

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

fxck commented Jan 28, 2017

I'm going to split it into more PRs.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants