Confirmation dialog adjustments #1331
Conversation
|
|
||
| this.confirmService.open(config).closed.subscribe((result: string) => { | ||
| this.action = 'You clicked \'' + result + '\''; | ||
| instance.confirmed.subscribe((result: any) => { |
There was a problem hiding this comment.
Is there a strong-type we can use for result?
There was a problem hiding this comment.
The emitter passes a strongly-typed response, but the consumer can use any if they like.
There was a problem hiding this comment.
Very true, I just don't want to encourage it in demo code. Wouldn't using the strongly-typed response be considered a best practice? I know for a fact that people look to the demo code as "the correct way to do things." In that case, it would be nice if the demo code represented best practices.
| @@ -0,0 +1,4 @@ | |||
| export interface SkyConfirmationDialogButtonConfig { | |||
| text?: string; | |||
There was a problem hiding this comment.
Should this be required since we probably aren't going to have buttons with no text?
There was a problem hiding this comment.
There are defaults for each of these properties, so they can remain optional for now.
| @@ -0,0 +1 @@ | |||
| export type SkyConfirmationDialogButtonAction = 'ok' | 'yes' | 'no' | 'cancel'; | |||
There was a problem hiding this comment.
One of our confirmation dialogs is going to have a Delete button so I'm a bit torn about these specific actions, but we could easily use something like 'yes' for our usage.
There was a problem hiding this comment.
These more-or-less serve as "ids" for the various buttons. Open to alternatives...
| export class SkyConfirmationDialogButton { | ||
| public text?: string; | ||
| public action?: SkyConfirmationDialogButtonAction; |
There was a problem hiding this comment.
Here, I feel like text and action are likely things that you'd consider required. Buttons need text, and they need some sort of action associated with them for when they are clicked.
There was a problem hiding this comment.
Those optional flags were a hold-over from the previous configuration implementation. Good catch.
| buttons: [ { text: '1' }, { text: '2' }, { text: '3', autofocus: true } ] | ||
| }; | ||
| buttons: [ | ||
| { text: '1' }, |
There was a problem hiding this comment.
If I'm setting up buttons like this, how do I know the action that would be associated with each button? Is that set based on type?
There was a problem hiding this comment.
It's based on the number of buttons.
There was a problem hiding this comment.
Which corresponds to the type, yes.
| } as SkyConfirmationDialogButton; | ||
| } | ||
|
|
||
| this.confirmed.emit(result.data); |
There was a problem hiding this comment.
I still feel that confirmed and closed should be completed after emitting a value. They should never fire more than once and should help reduce memory leaks from Subscriptions left open.
There was a problem hiding this comment.
Good point.
|
|
||
| @Injectable() | ||
| export class SkyConfirmationDialogInstance { | ||
| public closed = new EventEmitter<string>(); | ||
| public confirmed = new EventEmitter<SkyConfirmationDialogButton>(); |
There was a problem hiding this comment.
I know why this exists since you're trying to deprecate closed. I do wonder if people will wonder if this will only fire if the user clicked the 'ok' or 'yes' actions.
There was a problem hiding this comment.
I chose this naming convention because it mirror's vanilla JavaScript's confirm("some message") method. We can investigate if there's a better word for it...
|
Closing for now; will create a new pull request shortly. |
Addresses:
#1330
#1329
#1328
#1324
#1323
confirmedwhich returns an object representing the button clicked.buttonTypeproperty isn't exposed to the consumer)closedemitter.