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

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

Merged
merged 3 commits into from
Feb 6, 2017

Conversation

crisbeto
Copy link
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.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 18, 2016
@crisbeto
Copy link
Member Author

@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
Copy link
Member

clydin commented Dec 19, 2016

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
Copy link
Contributor

shlomiassaf commented Jan 7, 2017

@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'm the token master' }
  ]
});

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:

class SometDialog {
  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
Member

@shlomiassaf MdDialogRef 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
Member

@crisbeto WRT typings,

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

export class MdDialogData<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.ts
export const MD_DIALOG_DATA = new OpaqueToken('MdDialogData');
...
  get(token: any, notFoundValue?: any): any {
    if (token === MdDialogRef) {
      return this._dialogRef;
    } else if (token == MD_DIALOG_DATA) {
      return config.data;
    }

    return this._parentInjector.get(token, notFoundValue);
  }

Then the user would inject the data:

@Component({...})
export class UserProfileDialog(@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
Copy link
Member Author

crisbeto commented Jan 13, 2017

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
Member Author

@jelbourn Re-did this to use the OpaqueToken.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

| `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
Member

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']);
  }
}
```

Fixes angular#2181.
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Feb 4, 2017
@crisbeto
Copy link
Member Author

crisbeto commented Feb 4, 2017

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

@tinayuangao tinayuangao merged commit 29cbe61 into angular:master Feb 6, 2017
@sum0mer
Copy link

sum0mer commented Mar 1, 2017

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
Member Author

crisbeto commented Mar 1, 2017

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

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
action: merge The PR is ready for merge by the caretaker 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.

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