Alternative confirm component #1333
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1333 +/- ##
======================================
Coverage 100% 100%
======================================
Files 364 370 +6
Lines 6723 6796 +73
Branches 865 874 +9
======================================
+ Hits 6723 6796 +73
Continue to review full report at Codecov.
|
Blackbaud-SteveBrush
requested review from
Blackbaud-BobbyEarl and
Blackbaud-PaulCrowder
| <button | ||
| type="button" | ||
| class="sky-btn sky-btn-primary sky-confirm-btn-ok" | ||
| (click)="openConfirm(1)"> |
There was a problem hiding this comment.
I may be out in left field, but would it make sense to use the new type here, instead of a number?
There was a problem hiding this comment.
Not sure it's possible to include an enum here?
There was a problem hiding this comment.
The value SkyConfirmType.OK === 1, for example...
There was a problem hiding this comment.
This probably isn't a big deal in the visual tests, but I saw the same thing in demo. Instead of having one method and passing in the number, I think I'd just break them out into separate methods (openYesCancel(), openYesNoCancel() etc.) and then demonstrate the use of the enum within the body of the method.
| @@ -0,0 +1,5 @@ | |||
| export interface SkyConfirmButtonConfig { | |||
| action?: string; | |||
There was a problem hiding this comment.
action as a string will lead to some wonkiness where the user can configure the action to be something other than a SkyConfirmButtonAction. Someone might expect SkyConfirmButton.action to be a known value, but it would end up being set to something completely custom to the user.
There was a problem hiding this comment.
I understand your point, but this was done to allow the developer to write custom actions, as needed. @Blackbaud-PaulCrowder can confirm the need of the requirement.
There was a problem hiding this comment.
I'm totally fine with custom actions by the user. It just seems like maybe SkyConfirmationButtonAction shouldn't exist as a type. The union string type usually notifies to the developer and compiler that the only values this type can hold are those listed in the union. Allowing the user specify any string makes this no longer true and SkyConfirmButton.action is misrepresenting itself.
There was a problem hiding this comment.
@Blackbaud-SteveBrush Do we currently have a way for the consumer to specify a custom action on a button? I don't see that as a property on SkyConfirmButtonConfig.
| export class SkyConfirmInstance { | ||
| public closed = new EventEmitter<SkyConfirmCloseEventArgs>(); | ||
|
|
||
| public open( |
There was a problem hiding this comment.
Why does open exist on SkyConfirmInstance? The service itself has an open function that returns a SkyConfirmInstance. Why would we also have an open function on the instance that the user could call too?
There was a problem hiding this comment.
I noticed this, too. Seems like this logic could just exist inside SkyConfirmService.
Blackbaud-SteveBrush
dismissed
Blackbaud-PaulCrowder’s stale review
@Blackbaud-BobbyEarl approved the changes in @Blackbaud-PaulCrowder's absence.
Addresses:
#574
#1330
#1329
#1328
#1324
#1323