Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(tooltip): implement change detection #3380

Merged
merged 2 commits into from Oct 21, 2019
Merged

fix(tooltip): implement change detection #3380

merged 2 commits into from Oct 21, 2019

Conversation

peterblazejewicz
Copy link
Contributor

This fixes issues with custom tooltip class changes not being applied after
tooltip creation.

Fixes: #3335

@peterblazejewicz peterblazejewicz mentioned this pull request Sep 24, 2019
Closed
@codecov-io
Copy link

codecov-io commented Sep 24, 2019 •
edited

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3380      +/-   ##
==========================================
+ Coverage   91.05%   91.06%   +<.01%     
==========================================
  Files          95       95              
  Lines        2774     2776       +2     
  Branches      515      516       +1     
==========================================
+ Hits         2526     2528       +2     
  Misses        189      189              
  Partials       59       59
Flag Coverage Δ
#e2e 55.51% <33.33%> (-0.05%) ⬇️
#unit 88.13% <100%> (ø) ⬆️
Impacted Files Coverage Δ
src/tooltip/tooltip.ts 98.46% <100%> (+0.04%) ⬆️

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 7324083...4feb165. Read the comment docs.

@joergplewe
Copy link

Great news. Will it be backported to the v4 release of ng-bootstrap? (I'm stuck with Angular7)

maxokorokov
maxokorokov requested changes Sep 27, 2019
View reviewed changes
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Hey, @peterblazejewicz, thanks for this PR!

Please check my comments about CD.

Cheers,
Max

src/tooltip/tooltip.ts Outdated Show resolved Hide resolved
src/tooltip/tooltip.ts Outdated Show resolved Hide resolved
src/tooltip/tooltip.spec.ts Outdated Show resolved Hide resolved
src/tooltip/tooltip.spec.ts Outdated Show resolved Hide resolved
@maxokorokov maxokorokov added this to the 5.1.2 milestone Sep 27, 2019
maxokorokov
maxokorokov requested changes Oct 17, 2019
View reviewed changes
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

@peterblazejewicz thanks for the updates, however the code is still not covered by tests:

https://codecov.io/gh/ng-bootstrap/ng-bootstrap/pull/3380/diff

@peterblazejewicz
Copy link
Contributor Author

reworked, updated. Thanks!

peterblazejewicz and others added 2 commits October 21, 2019 11:38
@peterblazejewicz @maxokorokov
fix(tooltip): implement change detection
cd09dfc 
This fixes issues with custom tooltip class changes  not being applied after
tooltip creation.

Fixes: ng-bootstrap#3335
@maxokorokov
test(tooltip): add 'tooltipClass' change test
4feb165
maxokorokov
maxokorokov approved these changes Oct 21, 2019
View reviewed changes
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Hey, @peterblazejewicz!

Thanks for the update, however I made small changes:

  1. Changed the test to the more simple and conventional one: just check that [tooltipClass]="expression" is correctly applied
  2. In the code used isOpen() instead of this._windowRef
  3. firstChange check is redundant, because we have isOpen() check

Will merge when Travis is green.

@maxokorokov maxokorokov merged commit 72f1206 into ng-bootstrap:master Oct 21, 2019
@peterblazejewicz peterblazejewicz deleted the fix/3335 branch October 21, 2019 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxokorokov maxokorokov maxokorokov approved these changes

Assignees
No one assigned
Labels
component: tooltip needs: merging
Projects
None yet
Milestone
5.1.2
Development

Successfully merging this pull request may close these issues.

Tooltip does not react on changes of tooltipClass
4 participants
@peterblazejewicz @codecov-io @joergplewe @maxokorokov