Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Completing SkyModalInstance.closed upon close #1326

Conversation

Blackbaud-ScottFreeman
Copy link
Contributor

This will make it so users can freely subscribe to the closed event of a SkyModalInstance without needing to worry about unsubscribing since the EventEmitter will be completed upon close. Also, this ensures that there will only ever be a single closed event, which should be the case since a modal cannot close more than once.

@blackbaud-sky-savage
Copy link
Collaborator

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

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

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

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1326   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         370     370           
  Lines        6788    6789    +1     
  Branches      874     874           
======================================
+ Hits         6788    6789    +1
Impacted Files Coverage Δ
src/modules/modal/modal-instance.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 1248759...6b11a4b. Read the comment docs.

@Blackbaud-SteveBrush
Copy link
Member

Addresses: #1244

@blackbaud-sky-savage
Copy link
Collaborator

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

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

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

@Blackbaud-ScottFreeman
Copy link
Contributor Author

Was their some weird issue with the merge that would have caused visual tests to fail for a non-visual change?

@blackbaud-sky-savage
Copy link
Collaborator

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

Commit: 32c36a2
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/327265178

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

@blackbaud-sky-savage
Copy link
Collaborator

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

Commit: 6b11a4b
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/327328100

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

@Blackbaud-SteveBrush
Copy link
Member

@blackbaud-sky-savage retry

@blackbaud-sky-savage
Copy link
Collaborator

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

Commit: 6b11a4b
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/327407218

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

@Blackbaud-SteveBrush Blackbaud-SteveBrush merged commit 70331e2 into blackbaud:master Jan 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants