feat(dialog): allow for a config object to be passed on init #1679
Conversation
googlebot
added
the
cla: yes
| role: DialogRole = 'dialog'; | ||
|
|
||
| constructor(config?: MdDialogConfigObject) { | ||
| Object.assign(this, config); | ||
| } |
There was a problem hiding this comment.
This isn't necessary- TypeScript uses structural typing, so you can pass an object literal with the appropriate properties everything should work. What's missing is that the properties should be marked as optional, e.g.:
role?: DialogRole = 'dialog';We can mark everything in here as optional this way except for viewContainerRef, which is required until angular/angular#9293 is resolved.
There was a problem hiding this comment.
I've been trying this approach out, but it seems like the role stays undefined in this case. E.g. check the generated JS here.
There was a problem hiding this comment.
I see what you mean. I want to keep it such the preferred API is
dialog.open(SomeComponent, {
someConfig: true
});And all the defaults are applied.
How about just having a method in MdDialog like _applyConfigDefaults that takes the config and applies the default values for things that are unset?
There was a problem hiding this comment.
That sounds good. If we remove the MdDialogConfig class, it would be a breaking change, right?
There was a problem hiding this comment.
We don't have to remove the class; it will still be the type of the argument, people can just additionally use a matching literal.
crisbeto
force-pushed
the
dialog-config-object
branch
2 times, most recently
from
cd8b25e to
e6fe109
Compare
|
Updated with your feedback @jelbourn. |
| /** | ||
| * Applies default options to the dialog config. | ||
| * @param {MdDialogConfig} config Config to be modified. | ||
| * @returns The new configuration object. |
There was a problem hiding this comment.
Are the JSDoc types necessary here? I think the TS types should be enough.
There was a problem hiding this comment.
Good point. Removed the types and renamed the argument to match the other methods.
crisbeto
force-pushed
the
dialog-config-object
branch
2 times, most recently
from
ca8ec47 to
b4bd730
Compare
| private _applyConfigDefaults(dialogConfig: MdDialogConfig): MdDialogConfig { | ||
| let defaults: MdDialogConfig = { | ||
| role: 'dialog', | ||
| disableClose: false |
There was a problem hiding this comment.
Why not leave the defaults in MdDialogConfig and simply say new MdDialogConfig here?
Adds the ability to pass in an object literal, that matches the signature of `MdDialogConfig`, when opening a dialog.
crisbeto
force-pushed
the
dialog-config-object
branch
from
b4bd730 to
79147cf
Compare
|
Updated again. |
|
LGTM |
jelbourn
added
pr: lgtm
action: merge
…ar#1679) Adds the ability to pass in an object literal, that matches the signature of `MdDialogConfig`, when opening a dialog.
|
I have following error: this is using example from MdDialog readme: I'm using Angular 2.2.3 and material 2.0.0-alpha.10 |
|
It works for me @Nogostradamus, can you retry checking your Material version? |
|
Sorry, my bad. Now it works. I was still on experimental pizza v9 after an update. I had to removed caret from package.json "@angular/material": "^2.0.0-alpha.10" in order to update successfully. |
|
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. |
Adds the ability to pass in a configuration object when creating a new
MdDialogConfig. This should make working with a lot of dialog options a little easier.