add simple confirmation dialog component #1209
Conversation
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3f76d30 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 755cf45 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: c78a319 (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #1209 +/- ##
======================================
Coverage 100% 100%
======================================
Files 355 362 +7
Lines 6635 6704 +69
Branches 854 863 +9
======================================
+ Hits 6635 6704 +69
Continue to review full report at Codecov.
|
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6717c55 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 03f60c0 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0fd44c3 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 4cbabaf (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: fb2b728 (Please note that this is a fully automated comment.) |
| @@ -0,0 +1,5 @@ | |||
| export class ConfirmationDialogConfig { | |||
There was a problem hiding this comment.
Prefix with Sky, please.
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: e133baf (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3c7d854 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: ed6d69f (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 952b61f (Please note that this is a fully automated comment.) |
|
@blackbaud-sky-savage retry |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 952b61f (Please note that this is a fully automated comment.) |
| templateUrl: './confirmation-dialog-form.component.html', | ||
| styleUrls: ['./confirmation-dialog-form.component.scss'] | ||
| }) | ||
| export class SkyConfirmationDialogFormComponent implements OnInit { |
There was a problem hiding this comment.
I wouldn't think the suffix "form" is needed here, especially since you don't have just a SkyConfirmationDialogComponent. You can probably get away with removing the Form and -form suffixes, to avoid confusion.
| public instance: SkyModalInstance) {} | ||
|
|
||
| public ngOnInit() { | ||
| if (!this.context.type) { |
There was a problem hiding this comment.
This is more speculation, but I'm curious if the component should have to worry about the context's default values. Since you're preparing the config in the SkyConfirmationDialogInstance.open() method, maybe you could run the default-checks there?
Interested to hear your thoughts!
There was a problem hiding this comment.
My thought was that the component would check for all defaults and overrides so that anything that creates it wouldn't have to worry about checking the default values. I can't think of scenario now where the component might get created in a different way than through the service/instance, but if it were to then the default config checking might have to be done there too. If you think it makes more sense to move the checking to the SkyConfirmationDialogInstance.open() method, I'm perfectly fine with moving it there.
| { | ||
| text: SkyResources.getString('confirm_dialog_default_ok_text'), | ||
| autofocus: true, | ||
| style: 'sky-btn sky-btn-primary sky-dialog-btn-1' |
There was a problem hiding this comment.
I'm not 100% sold on the use of "style", here. At first glance, it appeared that the property would be setting inline CSS styles (much like ngStyle). You could go with the vanilla JavaScript classNames, but I'm open to your suggestions (https://developer.mozilla.org/en-US/docs/Web/API/Element/className).
Another idea would be to have a static buttonType property that would store the value as primary, default, or link, and then have your template govern the CSS class name, to separate the concerns a bit (the component doesn't really "care" about the class name, just what type it is).
<button
*ngFor="let btn of buttons"
type="button"
class="sky-btn sky-confirmation-dialog-btn sky-btn-{{ btn.buttonType }}"
(click)="instance.close(btn.text)"
[attr.autofocus]="btn.autofocus ? 'autofocus' : null">
{{ btn.text }}
</button>
There was a problem hiding this comment.
I like the idea of the buttonType property. For some reason, I had thought a button couldn't have both 'sky-btn' and 'sky-btn-link' or else the 'sky-btn-link' styling wouldn't show. But I see now that my thought was incorrect. I will replace the style property for buttonType. Thanks for the advice.
| @@ -0,0 +1,22 @@ | |||
| <sky-modal> | |||
There was a problem hiding this comment.
It's a good idea to wrap your template in a div with a specific class so that other components can easily target its contents, such as:
<div class="sky-confirmation-dialog">
// ...
</div>
| import { SkyConfirmationDialogButton } from './confirmation-dialog-form.component'; | ||
|
|
||
| export class SkyConfirmationDialogConfig { | ||
| public description: string; |
There was a problem hiding this comment.
Mind changing description to message, to better emulate the vanilla JavaScript's confirm method arguments? https://www.w3schools.com/jsref/met_win_confirm.asp
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 92ca371 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 7cb85ec (Please note that this is a fully automated comment.) |
|
@blackbaud-sky-savage retry |
1 similar comment
|
@blackbaud-sky-savage retry |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: a01e525 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 4ad136c (Please note that this is a fully automated comment.) |
|
@blackbaud-sky-savage retry |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 4ad136c (Please note that this is a fully automated comment.) |
|
@Blackbaud-AnthonyLopez This looks good to me! I'm going to have @blackbaud-johnly take a quick look at the docs portion, if he doesn't mind. |
| class="sky-btn sky-btn-primary sky-test-one-confirmation-dialog" | ||
| (click)="openOneBtnDialog()" | ||
| > | ||
| Open one button dialog |
There was a problem hiding this comment.
"one-button" should be hyphenated
| class="sky-btn sky-btn-primary sky-test-two-confirmation-dialog" | ||
| (click)="openTwoBtnDialog()" | ||
| > | ||
| Open two button dialog |
There was a problem hiding this comment.
"two-button" should be hyphenated
| class="sky-btn sky-btn-primary sky-test-three-confirmation-dialog" | ||
| (click)="openThreeBtnDialog()" | ||
| > | ||
| Open three button dialog |
There was a problem hiding this comment.
"three-button" should be hyphenated
| class="sky-btn sky-btn-primary sky-test-long-confirmation-dialog" | ||
| (click)="openLongDescriptionDialog()" | ||
| > | ||
| Open dialog long |
There was a problem hiding this comment.
Why do we put the adjective after the noun here? Should this be "Open long dialog"? And if so, what do we mean by "long"?
There was a problem hiding this comment.
@blackbaud-johnly I was copying an existing example from the error-visual.component.html file. By "long", I mean a description that spans multiple lines. Would "Open dialog with long description" be better?
There was a problem hiding this comment.
I think "Open dialog with long description sounds great.
| @@ -0,0 +1,16 @@ | |||
| <button type="button" class="sky-btn sky-btn-primary" (click)="openConfirmationDialog(1)"> | |||
| OK dialog | |||
There was a problem hiding this comment.
Should these labels be consistent with the terminology above? i.e., "One-button dialog," "Two-button dialog," and "Three-button dialog"? That strikes me as easier to process at a glance than "Yes no cancel dialog."
Also, if the fourth option here is a dialog with custom text on the buttons, is that what the "dialog long" option above is all about? If so, should "Open dialog long" be something like "Open dialog with custom button text"?
There was a problem hiding this comment.
@blackbaud-johnly These button labels do not necessarily go with the above terminology. I was basing this wording off of the confirm dialog enum types (in the "SkyConfirmationDialogType types" further up on this page), which there are 3 currently (OK, YesCancel, and YesNoCancel). The last button, "Custom button text dialog", is just supposed to demonstrate how a user can supply their own custom button text to the dialog to change the defaults. I am fine with changing the text still.
There was a problem hiding this comment.
OK. Sorry about the confusion on my end. And thanks for the clarification. ... I'd go ahead and change the first three labels as suggested, and I'd change the last to "Dialog with custom button text" if that works for you.
| name: 'Confirmation dialog', | ||
| icon: 'list-alt', | ||
| // tslint:disable-next-line | ||
| summary: `The confirmation dialog component allows users to confirm an action.`, |
There was a problem hiding this comment.
I think "allows users to confirm an action" should be something like "launches simple confirmation dialogs in a consistent way in SKY UX applications" since that's what we say on the help topic for the service. If we want to emphasize confirming the action, I'd add "to allow users to confirm actions" to the opening sentence of the the first paragraph on the help topic and change this summary to "launches simple confirmation dialogs to allow users to confirm actions."
There was a problem hiding this comment.
@blackbaud-johnly I am not sure if you want me to keep the beginning part, "The confirmation dialog component". Right now, I have it as "The confirmation dialog component launches simple confirmation dialogs to allow users to confirm actions." Is that okay? I also updated the text in the first paragraph of the help topic.
There was a problem hiding this comment.
That looks great to me. Sorry my initial feedback was unclear.
src/locales/resources_en_US.json
Outdated
| @@ -103,6 +103,22 @@ | |||
| "_description": "Label for the reset button to change the color back to the initial color", | |||
| "message": "Reset" | |||
| }, | |||
| "confirm_dialog_default_ok_text" : { | |||
| "_description": "Default text for confirm dialog's ok button", | |||
There was a problem hiding this comment.
"OK" should be capitalized
src/locales/resources_en_US.json
Outdated
| "message": "OK" | ||
| }, | ||
| "confirm_dialog_default_yes_text" : { | ||
| "_description": "Default text for confirm dialog's yes button", |
There was a problem hiding this comment.
I'd capitalize "Yes"
src/locales/resources_en_US.json
Outdated
| "message": "Yes" | ||
| }, | ||
| "confirm_dialog_default_no_text" : { | ||
| "_description": "Default text for confirm dialog's no button", |
There was a problem hiding this comment.
I'd capitalize "No"
src/locales/resources_en_US.json
Outdated
| "message": "No" | ||
| }, | ||
| "confirm_dialog_default_cancel_text" : { | ||
| "_description": "Default text for confirm dialog's cancel button", |
There was a problem hiding this comment.
I'd capitalize "Cancel"
|
@Blackbaud-AnthonyLopez Let me know if you have any questions regarding John's docs feedback. |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3e44829 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: d0179fe (Please note that this is a fully automated comment.) |
|
@Blackbaud-SteveBrush I made all of the updates from John's docs review. |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: a0f5c96 (Please note that this is a fully automated comment.) |
#574