Add help button to modal header #1144

Blackbaud-BrandonJones · 2017-09-29T18:05:02Z

Resolution for Issue #893

…p-modal-header

Blackbaud-BobbyEarl · 2017-09-29T18:42:50Z

In the documentations, it looks a little strange to me having the help icon render and having it wrapped in a <code> element. Is that the intended behavior?

Would it be possible to have the modal content display the same "Modal header help was invoked with the following help key: demo-key.html" message displayed in the modal, or some kind of visual feedback. I'm worried by nothing actually happening, some may think it's broken (without knowing to look at the console).

blackbaud-sky-savage · 2017-09-29T18:58:37Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: e1d4ce5
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/281399694

(Please note that this is a fully automated comment.)

codecov-io · 2017-09-29T18:59:46Z

Codecov Report

Merging #1144 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1144   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         355     355           
  Lines        6578    6590   +12     
  Branches      842     842           
======================================
+ Hits         6578    6590   +12

Impacted Files	Coverage Δ
src/modules/modal/modal-configuration.ts	`100% <ø> (ø)`	⬆️
src/modules/modal/modal-host.service.ts	`100% <100%> (ø)`	⬆️
src/modules/modal/modal-instance.ts	`100% <100%> (ø)`	⬆️
src/modules/modal/modal.component.ts	`100% <100%> (ø)`	⬆️
src/modules/modal/modal-host.component.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 569fece...1ac1dfa. Read the comment docs.

blackbaud-sky-savage · 2017-09-29T19:25:32Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 08da56c
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/281399727

(Please note that this is a fully automated comment.)

…d, removed code wrapping around the ? icon

…aud-BrandonJones/skyux2 into feature-help-modal-header

…p-modal-header

blackbaud-sky-savage · 2017-09-29T20:59:45Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 76f33f2
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/281442527

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-29T21:27:41Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: bcf506e
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/281442876

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-29T21:55:18Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 9ea609c
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/281465982

(Please note that this is a fully automated comment.)

Blackbaud-BrandonJones · 2017-09-29T22:00:42Z

@Blackbaud-BobbyEarl Implemented your suggestions!

Blackbaud-BrandonJones · 2017-09-29T22:01:16Z

Also, I am unable to add reviewers here, could/should someone also add @Blackbaud-PaulCrowder on this?

blackbaud-sky-savage · 2017-10-03T17:42:48Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 12eb50d
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/282833908

(Please note that this is a fully automated comment.)

…p-modal-header

…aud-BrandonJones/skyux2 into feature-help-modal-header

blackbaud-sky-savage · 2017-10-03T20:02:13Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: a672032
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/282886201

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-10-03T20:29:06Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 705a9a6
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/282895262

(Please note that this is a fully automated comment.)

Blackbaud-SteveBrush · 2017-10-04T16:51:32Z

src/modules/modal/modal.component.html

@@ -29,6 +29,9 @@
        <ng-content select="sky-modal-header"></ng-content>
      </div>
      <div class="sky-modal-header-buttons">
+        <button *ngIf="helpKey !== undefined" type="button" class="sky-btn" name="help-button" [attr.aria-label]="'open_help' | skyResources" (click)="helpButtonClick()">


Is the !== undefined needed here? Isn't *ngIf="helpKey" sufficient?

It should be sufficient, the difference would be that ngIf would be getting a direct boolean value by evaluating the expression, instead of an interpolated undefined vs something exists check. I am fine with either :) depends on your standards willing to adjust.

Blackbaud-SteveBrush · 2017-10-04T16:52:27Z

src/modules/modal/modal-host.component.ts

@@ -74,6 +74,10 @@ export class SkyModalHostComponent {
      modalComponentRef.destroy();
    }

+    hostService.openHelp.subscribe((helpKey?: string) => {


I just noticed that none of these subscriptions are unsubscribed during ngOnDestroy. Do you mind adding this to the component to see if it works?

Blackbaud-SteveBrush · 2017-10-04T16:55:49Z

src/modules/modal/modal-instance.ts

@@ -27,6 +28,10 @@ export class SkyModalInstance {
    this.closeModal('save', result);
  }

+  public openHelp(helpKey?: string) {
+    this.helpInvoked.emit(helpKey);


I'd prefer the public method's name was more similar to the emitter (this.helpOpened, or vice versa).

…p-modal-header

blackbaud-sky-savage · 2017-10-04T18:04:56Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 3c72e95
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/283300709

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-10-04T18:19:05Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 971e631
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/283302020

(Please note that this is a fully automated comment.)

…aud-BrandonJones/skyux2 into feature-help-modal-header

blackbaud-sky-savage · 2017-10-04T18:47:19Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 93217ce
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/283313342

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-10-04T19:17:06Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 5e5f19b
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/283319227

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-10-04T19:44:21Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: ae134e7
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/283324364

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-10-04T20:13:22Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 1ac1dfa
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/283346305

(Please note that this is a fully automated comment.)

Blackbaud-BrandonJones added 9 commits September 28, 2017 16:15

added the header icon to the modal

c91b21a

Merge branch 'master' of github.com:blackbaud/skyux2 into feature-hel…

d0b09e4

…p-modal-header

tests

ea716d5

added test, clearing old instance after open

e108ae9

created demo and documents

e364107

visual tests for help header

0e0dca0

re-worded to allow for line length

e1d4ce5

Merge branch 'master' into feature-help-modal-header

08da56c

Merge branch 'master' of github.com:blackbaud/skyux2 into feature-hel…

b36a567

…p-modal-header

blackbaud-brandonhare requested review from Blackbaud-SteveBrush and Blackbaud-BobbyEarl September 29, 2017 18:06

Blackbaud-BrandonJones changed the title ~~Feature help modal header~~ Add help button to modal header Sep 29, 2017

Blackbaud-BrandonJones added 4 commits September 29, 2017 16:06

better visual representation of the modal with a helpKey being clicke…

6c9edec

…d, removed code wrapping around the ? icon

Merge branch 'feature-help-modal-header' of https://github.com/Blackb…

bf485af

…aud-BrandonJones/skyux2 into feature-help-modal-header

Merge branch 'master' of github.com:blackbaud/skyux2 into feature-hel…

76f33f2

…p-modal-header

removed extra space

bcf506e

Merge branch 'master' into feature-help-modal-header

9ea609c

Merge branch 'master' into feature-help-modal-header

12eb50d

Blackbaud-BrandonJones added 2 commits October 3, 2017 15:27

Merge branch 'master' of github.com:blackbaud/skyux2 into feature-hel…

5e11169

…p-modal-header

Merge branch 'master' into feature-help-modal-header

a672032

Merge branch 'feature-help-modal-header' of https://github.com/Blackb…

705a9a6

…aud-BrandonJones/skyux2 into feature-help-modal-header

blackbaud-brandonhare requested a review from Blackbaud-PaulCrowder October 4, 2017 11:52

Blackbaud-SteveBrush reviewed Oct 4, 2017

View reviewed changes

Blackbaud-BrandonJones added 2 commits October 4, 2017 13:40

responding to feedback

971e631

Merge branch 'master' of github.com:blackbaud/skyux2 into feature-hel…

3c72e95

…p-modal-header

unsubscribing

93217ce

Blackbaud-SteveBrush and others added 3 commits October 4, 2017 14:26

Merge branch 'master' into feature-help-modal-header

5e5f19b

unsubbing

81b3870

Merge branch 'feature-help-modal-header' of https://github.com/Blackb…

ae134e7

…aud-BrandonJones/skyux2 into feature-help-modal-header

removed unsub stuff

1ac1dfa

Blackbaud-SteveBrush approved these changes Oct 4, 2017

View reviewed changes

Blackbaud-SteveBrush merged commit f3140ba into blackbaud:master Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add help button to modal header #1144

Add help button to modal header #1144

Blackbaud-BrandonJones commented Sep 29, 2017 •

edited

Loading

Blackbaud-BobbyEarl commented Sep 29, 2017 •

edited

Loading

blackbaud-sky-savage commented Sep 29, 2017

codecov-io commented Sep 29, 2017 •

edited

Loading

blackbaud-sky-savage commented Sep 29, 2017

blackbaud-sky-savage commented Sep 29, 2017

blackbaud-sky-savage commented Sep 29, 2017

blackbaud-sky-savage commented Sep 29, 2017

Blackbaud-BrandonJones commented Sep 29, 2017

Blackbaud-BrandonJones commented Sep 29, 2017

blackbaud-sky-savage commented Oct 3, 2017

blackbaud-sky-savage commented Oct 3, 2017

blackbaud-sky-savage commented Oct 3, 2017

Blackbaud-SteveBrush Oct 4, 2017

Blackbaud-BrandonJones Oct 4, 2017

Blackbaud-SteveBrush Oct 4, 2017

Blackbaud-SteveBrush Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

Add help button to modal header #1144

Add help button to modal header #1144

Conversation

Blackbaud-BrandonJones commented Sep 29, 2017 • edited Loading

Blackbaud-BobbyEarl commented Sep 29, 2017 • edited Loading

blackbaud-sky-savage commented Sep 29, 2017

codecov-io commented Sep 29, 2017 • edited Loading

Codecov Report

blackbaud-sky-savage commented Sep 29, 2017

blackbaud-sky-savage commented Sep 29, 2017

blackbaud-sky-savage commented Sep 29, 2017

blackbaud-sky-savage commented Sep 29, 2017

Blackbaud-BrandonJones commented Sep 29, 2017

Blackbaud-BrandonJones commented Sep 29, 2017

blackbaud-sky-savage commented Oct 3, 2017

blackbaud-sky-savage commented Oct 3, 2017

blackbaud-sky-savage commented Oct 3, 2017

Blackbaud-SteveBrush Oct 4, 2017

Choose a reason for hiding this comment

Blackbaud-BrandonJones Oct 4, 2017

Choose a reason for hiding this comment

Blackbaud-SteveBrush Oct 4, 2017

Choose a reason for hiding this comment

Blackbaud-SteveBrush Oct 4, 2017

Choose a reason for hiding this comment

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

blackbaud-sky-savage commented Oct 4, 2017

Blackbaud-BrandonJones commented Sep 29, 2017 •

edited

Loading

Blackbaud-BobbyEarl commented Sep 29, 2017 •

edited

Loading

codecov-io commented Sep 29, 2017 •

edited

Loading