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 result to MdDialogClose directive #4332

Merged
merged 1 commit into from
May 22, 2017

Conversation

nallwhy
Copy link
Contributor

@nallwhy nallwhy commented Apr 30, 2017

MdDialog has MdDialogClose to make it easier to structure a dialog.
But as MdDialogClose doesn't return any value, it's imposibble to use MdDialogClose if you want to get any return value when MdDialog is closed.
So @Input('md-dialog-close')result is added to MdDialogClose to solve it.

no breaking changes

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 30, 2017
@nallwhy nallwhy changed the title feat(dialog): add result to MdDialogClose directive feat(dialog): add result to MdDialogClose directive May 1, 2017
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.

Looks good, just needs a mention in the docs

@@ -17,6 +17,9 @@ export class MdDialogClose {
/** Screenreader label for the button. */
@Input('aria-label') ariaLabel: string = 'Close dialog';

/** Result to return to the dialog opener. */
@Input('md-dialog-close') result: any;
Copy link
Member

Choose a reason for hiding this comment

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

Can you also update the overview in dialog.md to mention this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn I've done it! The tests below failed but the errors are not related to this commit.
What should I do?

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work in compatibility mode. We need something for mat-dialog-close too.

@nallwhy nallwhy force-pushed the add_result_to_md-dialog-close branch 2 times, most recently from e56b469 to 66166c4 Compare May 3, 2017 12:38
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.

Just a couple comments on the docs

<md-dialog-actions>
<button md-button [md-dialog-close]="true">Yes</button>
<button md-button [md-dialog-close]="false">No</button>
</md-dialog-actions>
Copy link
Member

Choose a reason for hiding this comment

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

One example should show the button without the value and one with, e.g.:

<h2 md-dialog-title>Delete all</h2>
<md-dialog-content>Are you sure?</md-dialog-content>
<md-dialog-actions>
  <button md-button md-dialog-close>Yes</button>
  <!-- Can optionally provide a result for the closing dialog. -->
  <button md-button [md-dialog-close]="false">No</button>
</md-dialog-actions>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

| `md-dialog-title` | \[Attr] Dialog title, applied to a heading element (e.g., `<h1>`, `<h2>`) |
| `<md-dialog-content>` | Primary scrollable content of the dialog |
| `<md-dialog-actions>` | Container for action buttons at the bottom of the dialog |
| `md-dialog-close` | \[Attr] Added to a `<button>`, makes the button close the dialog and return the binded value on click|
Copy link
Member

Choose a reason for hiding this comment

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

\[Attr] Added to a `<button>`, makes the button close the dialog with an optional result from the bound value.

@nallwhy nallwhy force-pushed the add_result_to_md-dialog-close branch from 66166c4 to 0643a2e Compare May 3, 2017 17:23
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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 3, 2017
@devversion devversion removed the action: merge The PR is ready for merge by the caretaker label May 8, 2017
@nallwhy nallwhy force-pushed the add_result_to_md-dialog-close branch 2 times, most recently from 83d193f to b8365a8 Compare May 8, 2017 23:12
@nallwhy
Copy link
Contributor Author

nallwhy commented May 9, 2017

@devversion I've added mat-dialog-close.

/** Result for compatibility mode */
@Input('mat-dialog-close')
get _matResult(): any { return this.result; }
set _matResult(r: any) { this.result = r; }
Copy link
Member

@devversion devversion May 9, 2017

Choose a reason for hiding this comment

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

I think we can also have just the set here

/** Dialog close input for compatibility mode. */
@Input('mat-dialog-close') set _matDialogClose(value: any) { this.dialogResult = value; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devversion You are right. I've changed that.

@nallwhy nallwhy force-pushed the add_result_to_md-dialog-close branch 3 times, most recently from e1de055 to de86e14 Compare May 9, 2017 23:17
MdDialog has MdDialogClose to make it easier to structure a dialog.
But as MdDialogClose doesn't return any value, it's imposibble to use MdDialogClose if you want to get any return value when MdDialog is close.
So `@Input('md-dialog-close')result` is added to MdDialogClose to solve it.

no breaking changes
@nallwhy nallwhy force-pushed the add_result_to_md-dialog-close branch from de86e14 to b85fa23 Compare May 16, 2017 00:34
@nallwhy
Copy link
Contributor Author

nallwhy commented May 18, 2017

@devversion Any updates?

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label May 19, 2017
@devversion
Copy link
Member

@nallwhy Sorry I forgot about it. It's now merge ready and should be merged soon! Thanks again

@nallwhy
Copy link
Contributor Author

nallwhy commented May 19, 2017

@devversion Thanks!

@tinayuangao tinayuangao merged commit c45dee2 into angular:master May 22, 2017
@frederikaalund
Copy link

Note that this changed the default value from undefined to the empty string. Example:

<button md-button md-dialog-close>No</button>

I ran into this issue when upgrading angular/material2. The fix was trivial:

<button md-button [md-dialog-close]="undefined">No</button>

Just leaving this here for others to see.

@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.

6 participants