Skip to content

feat(dialog): add a config option for passing in data - #2266

Merged
tinayuangao merged 3 commits into
angular:masterfrom
crisbeto:2181/dialog-data
Feb 6, 2017
Merged

feat(dialog): add a config option for passing in data#2266
tinayuangao merged 3 commits into
angular:masterfrom
crisbeto:2181/dialog-data

Conversation

@crisbeto

Copy link
Copy Markdown
Member

Adds the ability to pass in data to a dialog via the data property. While this was previously possible through the dialogRef.componentInstance.someUserSuppliedProperty, it wasn't the most intuitive. The new approach looks like this:

dialog.open(SomeDialog, {
data: {
hello: 'world'
}
});
class SometDialog {
constructor(data: MdDialogData) {
console.log(data['hello']);
}
}

Fixes#2181.

@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 18, 2016
@crisbeto

Copy link
Copy Markdown
MemberAuthor

@jelbourn I'm not huge fan of having to access the properties on the data object through bracket notation, but I'm not sure how we could it better. Of course, people can still cast it to the proper type.

@clydin

clydin commented Dec 19, 2016

Copy link
Copy Markdown
Member

An option could be to have the MdDialogRef contain the data property with an added second generic parameter to type the property. open would probably need the generic type as well for completeness.

@shlomiassaf

shlomiassaf commented Jan 7, 2017

Copy link
Copy Markdown
Contributor

@crisbeto@jelbourn This is not what I suggested in #2181

Let the user pass providers and you add them into the injector.

When you instantiate the component the user has provided you already add something to the injector the MdDialogRef since it is unique for each dialog opened.

This is what I had in mind:

dialog.open(SomeDialog,{providers : [{provider: MyPreciousToken,useValue: 'I'mthetokenmaster' }]});

You can of course use @Injectables there as well, not just tokens.
This is quite common, and supports all behaviours an angular user should know...

The current PR doesn't change much, you still don't get natural type support, if you want type support you need to do:

classSometDialog{constructor(@Inject(MdDialogData)data: MyHelloDataModel){console.log(data.hello);}}

If you insist on going that road I suggest changing MdDialogRef to MdDialogRef<T>, this way we don't need the extra token and MdDialogRef will carry the data, the data property will be of type T...

But again, i'm pretty sure once more people use it that you will get a request for provider injection (or injector replacement) since the user can't control it.

I have a modal library for angular from alpha 17, a lot of people use the provider injection.

@jelbourn

Copy link
Copy Markdown
Contributor

@shlomiassafMdDialogRef is already a generic, where T is the type of the component inside the dialog. We could potentially change it to MdDialogRef<T, D>, where D is the type of the passed in data, but that then begs the questions- what about the return value? Then we'd have MdDialogRef<T, D, R>, which I think everyone would find rather cumbersome.

@jelbourn

Copy link
Copy Markdown
Contributor

@crisbeto WRT typings,

We could make MdDialogData generic with exactly one property, e.g.,

exportclassMdDialogData<T>{value: T;}

People could then inject, say, MdDialogData<User>, pull out the value and go about their day. However, MdDialogData doesn't really do anything here other than act as an injection token for the data- so why not just make an injection token?

// In dialog-injector.tsexportconstMD_DIALOG_DATA=newOpaqueToken('MdDialogData');
...
get(token: any,notFoundValue?: any): any {if(token===MdDialogRef){returnthis._dialogRef;}elseif(token==MD_DIALOG_DATA){returnconfig.data;}returnthis._parentInjector.get(token,notFoundValue);}

Then the user would inject the data:

@Component({...})exportclassUserProfileDialog(@Inject(MD_DIALOG_DATA)user: User){// ...}

The data on the MdDialogConfig would stay as an any since there's no way we could enforce the type through injection anyway.

@crisbeto

crisbeto commented Jan 13, 2017

Copy link
Copy Markdown
MemberAuthor

Yeah, something similar to the OpaqueToken option was what I was going for initially, but didn't figure out how to do properly. I'll rework the PR.

@crisbeto

Copy link
Copy Markdown
MemberAuthor

@jelbourn Re-did this to use the OpaqueToken.

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadsrc/lib/dialog/README.md Outdated
| `viewContainerRef: ViewContainerRef` | The view container ref to attach the dialog to. Optional. |
| `role?: DialogRole = 'dialog'` | The ARIA role of the dialog element. Possible values are `dialog` and `alertdialog`. |
| `disableClose?: boolean = false` | Whether to prevent the user from closing a dialog by clicking on the backdrop or pressing escape. |
| `data?: { [key: string]: any }` | Data that will be injected into the dialog as via the `MD_DIALOG_DATA` token. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The type of data should be any

Adds the ability to pass in data to a dialog via the `data` property. While this was previously possible through the `dialogRef.componentInstance.someUserSuppliedProperty`, it wasn't the most intuitive. The new approach looks like this:
```
dialog.open(SomeDialog, {
data: {
hello: 'world'
}
});
class SometDialog {
constructor(data: MdDialogData) {
console.log(data['hello']);
}
}
```
Fixesangular#2181.
@crisbetocrisbeto added the action: merge The PR is ready for merge by the caretaker label Feb 4, 2017
@crisbeto

Copy link
Copy Markdown
MemberAuthor

This should be good to go. The test failure is due to #2903

@tinayuangao
tinayuangao merged commit 29cbe61 into angular:masterFeb 6, 2017
@sum0mer

Copy link
Copy Markdown

Hi, I tried to 'import {MdDialog, MdDialogRef, MdDialogData} from '@angular/material';', but got [workspace/angular2/node_modules/@angular/material/index"' has no exported member 'MdDialogData'] compile error. Any suggestion to make it work? Thanks!

@crisbeto

Copy link
Copy Markdown
MemberAuthor

We didn't end up using MdDialogData@sum0mer. You can see an example of how it should be used in the demo app: https://github.com/angular/material2/blob/master/src/demo-app/dialog/dialog-demo.ts#L80

@angular-automatic-lock-bot

Copy link
Copy Markdown

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-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 6, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MdDialog communication (i.e: passing data to MdDialog)

7 participants

@crisbeto@clydin@shlomiassaf@jelbourn@sum0mer@googlebot@tinayuangao