Skip to content

Conversation

@j0shko
Copy link
Contributor

@j0shko j0shko commented Mar 4, 2019

Closes #40

@tnicola
Copy link
Owner

tnicola commented Mar 4, 2019

Hi @j0shko ,
The PR is fine, thanks for contributing! Could you please add a couple of unit tests to check the class is added correctly in both the component and the backdrop? Also, the build is not passing since a unit test is failing.

If you want I can do all the modifications.

@j0shko
Copy link
Contributor Author

j0shko commented Mar 4, 2019

Hi @tnicola ,
Thank you for the quick response. 😄
I'll add tests and fix the build, it's not a problem.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   92.77%   92.78%   +<.01%     
==========================================
  Files          16       16              
  Lines         831      832       +1     
  Branches      105      105              
==========================================
+ Hits          771      772       +1     
  Misses         27       27              
  Partials       33       33
Impacted Files Coverage Δ
src/lib/src/services/joyride-backdrop.service.ts 96.96% <100%> (+0.02%) ⬆️

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 e7ba0ee...ec0d386. Read the comment docs.

@j0shko
Copy link
Contributor Author

j0shko commented Mar 6, 2019

Hi @tnicola,
I switched from classes to ids, and added tests. Can you please check if tests are in correct place?

@tnicola tnicola merged commit 55c93bd into tnicola:master Mar 6, 2019
@tnicola
Copy link
Owner

tnicola commented Mar 6, 2019

Everything's fine, merged!Thanks @j0shko !

@tnicola
Copy link
Owner

tnicola commented Mar 6, 2019

Released in the new version 2.2.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants