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

Implemented flyout closure on navigation. #22

Merged
merged 4 commits into from
Mar 20, 2019

Conversation

Blackbaud-TrevorBurch
Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch commented Feb 15, 2019

Addresses blackbaud/skyux-flyout#20

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #22 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage   99.71%   99.72%   +<.01%     
==========================================
  Files          10       10              
  Lines         351      358       +7     
  Branches       52       54       +2     
==========================================
+ Hits          350      357       +7     
  Misses          1        1
Impacted Files Coverage Δ
src/app/public/modules/flyout/flyout.service.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 2457709...ea2f980. Read the comment docs.

Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small change

src/app/public/modules/flyout/flyout.service.ts Outdated Show resolved Hide resolved
* added navigate button to demo

* reverted old code

* removed baselines
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done!

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a problem when we added this same feature to the modal host component. People that were testing their components (that also implemented the modal component/service) were getting new errors saying Router is not provided. It might be worth double-checking that a component fixture that implements the flyout (in another consuming SPA) doesn't throw this error during skyux test.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since we're removing the screenshots, now would be a good time to implement the screenshotName property in the e2e tests. I'm seeing the following warnings in the logs:

A unique screenshot name was not provided!
 We'll use "body0" as a stand-in, but this can cause problems if you decide to change the order of the specs in the future. To set the screenshot name for each test, add a config object to the matcher:
 `expect('.foobar').toMatchBaselineScreenshot(done, { screenshotName: 'unique-name' });`

@Blackbaud-TrevorBurch
Copy link
Member Author

Good call on the screenshot names @Blackbaud-SteveBrush that is done :)

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch merged commit db77050 into master Mar 20, 2019
@Blackbaud-TrevorBurch Blackbaud-TrevorBurch deleted the closing-on-naviagtion branch March 20, 2019 17:10
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.

3 participants