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): not working properly with ChangeDetectionStrategy.OnPush #2721

Merged
merged 4 commits into from
Jan 31, 2017
Merged

fix(tooltip): not working properly with ChangeDetectionStrategy.OnPush #2721

merged 4 commits into from
Jan 31, 2017

Conversation

thomaspink
Copy link
Contributor

fixes #2713

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 19, 2017
thomaspink
thomaspink commented Jan 19, 2017
View reviewed changes
src/lib/tooltip/tooltip.ts

// Call change detection manually so tooltip will also work
// if any parent component has set the ChangeDetectionStrategy to OnPush
this._ref.detectChanges();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn is there a way to find out if a parent component has set the ChangeDetectionStrategy to OnPush or not so we can skip this line?

devversion
devversion reviewed Jan 19, 2017
View reviewed changes
src/lib/tooltip/tooltip.ts

// Call change detection manually so tooltip will also work
// if any parent component has set the ChangeDetectionStrategy to OnPush
this._ref.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

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

I think rather than always triggering a ChangeDetection we should just mark for a new check.

We are doing this in other components as well.

https://angular.io/docs/js/latest/api/core/index/ChangeDetectorRef-class.html

src/lib/tooltip/tooltip.ts
@@ -286,7 +287,7 @@ export class TooltipComponent {
/** Subject for notifying that the tooltip has been hidden from the view */
private _onHide: Subject<any> = new Subject();

constructor(@Optional() private _dir: Dir) {}
constructor(@Optional() private _dir: Dir, private _ref: ChangeDetectorRef) {}
Copy link
Member

Choose a reason for hiding this comment

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

_ref is not very precise. It should be _changeDetectorRef

@thomaspink
Copy link
Contributor Author

@devversion getting error in e2e test - maybe restart?

gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now

@devversion
Copy link
Member

devversion commented Jan 19, 2017 •
edited
Loading

@thomaspink Done. Never saw such an error on the CI...

Also what about adding a test for testing the OnPush strategy?

@thomaspink
Copy link
Contributor Author

@devversion seemed to work now - strange

Thanks

@thomaspink
Copy link
Contributor Author

@devversion trying to figure out whats happening here #2733 so i can submit test cases for this PR

@thomaspink
Copy link
Contributor Author

Added tests

@jelbourn jelbourn assigned andrewseguin and unassigned jelbourn Jan 24, 2017
jelbourn
jelbourn reviewed Jan 27, 2017
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple minor comments

src/lib/tooltip/tooltip.spec.ts
@@ -76,6 +90,43 @@ describe('MdTooltip', () => {
expect(tooltipDirective._tooltipInstance).toBeNull();
}));

it('should show and hide the tooltip if changeDetection is OnPush', fakeAsync(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this test to a separate describe block called "with OnPush" as a sibling to the existing "basic usage" block? Then you can do the createComponent in the beforeEach.

src/lib/tooltip/tooltip.spec.ts
tick(500);

// Make sure tooltip is shown to the user and animation has finished
const tooltipElement = <HTMLElement>overlayContainerElement.querySelector('.md-tooltip');
Copy link
Member

Choose a reason for hiding this comment

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

Prefer the as HTMLElement syntax

@thomaspink
Copy link
Contributor Author

@jelbourn implemented as suggested.

jelbourn
jelbourn approved these changes Jan 27, 2017
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 27, 2017
@kara kara merged commit 632b964 into angular:master Jan 31, 2017
@thomaspink thomaspink deleted the tooltip-on-push branch February 1, 2017 11:51
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion left review comments

@jelbourn jelbourn jelbourn approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees

@andrewseguin andrewseguin

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mdTooltip is not working properly with ChangeDetectionStrategy.OnPush
6 participants
@thomaspink @devversion @jelbourn @kara @googlebot @andrewseguin