feat(tooltip): add tooltip animations #1644

andrewseguin · 2016-10-28T22:54:02Z

Adding animations to the tooltip

googlebot · 2016-10-28T22:54:03Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-10-28T22:54:03Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-10-28T23:16:13Z

CLAs look good, thanks!

googlebot · 2016-10-28T23:16:13Z

CLAs look good, thanks!

jelbourn · 2016-10-29T00:53:10Z

src/lib/tooltip/tooltip.ts


 export type TooltipPosition = 'before' | 'after' | 'above' | 'below';

+/** Time in ms to delay before changing the tooltip visibility to hidden */
+export const MATERIAL_TOOLTIP_HIDE_DELAY  = 1500;


Just TOOLTIP_HIDE_DELAY; the context for "material" is implied

jelbourn · 2016-10-29T00:56:08Z

src/lib/tooltip/tooltip.ts

-      this._overlayRef = this._overlay.create(config);
+  /** Shows/hides the tooltip */
+  toggle(): void {
+    if (this._isTooltipVisible()) {


Could use a ternary:

this._isTooltipVisible() ? this.hide() : this.show();

jelbourn · 2016-10-29T01:15:50Z

src/lib/tooltip/tooltip.ts

+  /** Property watched by the animation framework to show or hide the tooltip */
+  _visibility: TooltipVisibility;
+
+  /** Set to true when interactions on the page should close the tooltip */


Whether interactions on the page should close the tooltip

jelbourn · 2016-10-29T01:28:51Z

src/lib/tooltip/tooltip.ts

+    return this._onHide.asObservable();
+  }
+
+  /** Returns true if the tooltip is being displayed */


Whether the tooltip is being displayed

jelbourn · 2016-10-29T01:29:47Z

src/lib/tooltip/tooltip.ts

-})
-export class TooltipComponent {
-  message: string;
+  _handleBodyInteraction(): void {


Add comment with explanation for this behavior pointing to the spec.

jelbourn · 2016-10-29T01:32:46Z

src/lib/tooltip/tooltip.ts

 })
 export class MdTooltip {
-  visible: boolean = false;
+  _overlayRef: OverlayRef;
+  _tooltipRef: TooltipComponent;


Should be named something like _tooltipInstance
(though I'm thinking we should rename TooltipComponent to something like TooltipContainer, in which case this would be the more sensible _tooltipContainerInstance)

jelbourn · 2016-10-29T19:37:24Z

src/lib/tooltip/tooltip.ts

+    this._hideTimeoutId = setTimeout(() => {
+      this._visibility = 'hidden';
+      this._closeOnInteraction = false;
+    }, delay);


Unit test for show() being called while hide() is in-progress?

andrewseguin · 2016-10-31T18:47:57Z

Comments addressed, added test for showing before the hide delay. Ready for review.

Would like to add tests that trigger after the animation is completed but it doesn't seem to be firing.

jelbourn · 2016-10-31T20:06:44Z

src/lib/tooltip/tooltip.spec.ts


-describe('MdTooltip', () => {
+fdescribe('MdTooltip', () => {


fdescribe and fit

jelbourn · 2016-10-31T20:07:54Z

src/lib/tooltip/tooltip.ts

-    ViewContainerRef,
-    ChangeDetectorRef
+    NgModule, ModuleWithProviders, Component, Directive, Input, ElementRef, ViewContainerRef,
+    style, trigger, state, transition, animate, AnimationTransitionEvent, NgZone


These are generally wrapped to be one-per line; WebStorm's Optimize imports command should do this automatically (though it always removes the trailing comma and uses 4 spaces).

andrewseguin · 2016-10-31T21:14:24Z

Thanks for the comments, I removed the fdescribe and updated the imports

andrewseguin · 2016-10-31T21:58:19Z

Realized I needed a detectChanges to trigger the done animation and then used fixture.whenStable() to test for the observable trigger that follows. Change is ready for review

jelbourn

Just a couple more things

jelbourn · 2016-11-01T00:17:55Z

src/lib/tooltip/tooltip.spec.ts

+      expect(tooltipDirective._isTooltipVisible()).toBe(false);
+
+      // On animation complete, should expect that the tooltip has been detached.
+      fixture.whenStable().then(() => {


You shouldn't need whenStable with fakeAsync, since you're in control of ticking the clock.
(unless there's something special going on with ngAnimate that I don't know about)

jelbourn · 2016-11-01T00:29:40Z

src/lib/tooltip/tooltip.ts

-    if (this._overlayRef) {
-      this._changeDetectionRef.detectChanges();
-      this._overlayRef.updatePosition();
+  _handleBodyInteraction(): void {


I thought about this a little more, and started worry that this will be a performance problem- every instance of tooltip will add a handler that will run on every click in the application, which could add up to a slowdown. I'm thinking now that it would be better to directly add and remove the click handler when the tooltip is shown/hidden.

In practice, this is happens because the parent will create an overlay w/ tooltip on show, and detach the tooltip when the tooltip finishes the hide animation. Would you like this behavior to be implemented explicitly in the tooltip component?

I think moving the handler to TooltipComponent would solve the issue

jelbourn · 2016-11-01T21:21:31Z

LGTM

rolandjitsu · 2016-11-10T09:42:14Z

src/lib/tooltip/tooltip.spec.ts

 import {Component, DebugElement} from '@angular/core';
 import {By} from '@angular/platform-browser';
-import {TooltipPosition, MdTooltip} from './tooltip';
+import {TooltipPosition, MdTooltip, TOOLTIP_HIDE_DELAY, MdTooltipModule} from './tooltip';


How can you globally configure TOOLTIP_HIDE_DELAY?

jelbourn · 2016-11-10T17:07:33Z

@rolandjitsu can you open an issue requesting that?

rolandjitsu · 2016-11-10T17:48:15Z

@jelbourn sure thing 👍

andrewseguin · 2016-11-10T17:54:09Z

Check out #1806 to track the issue

rolandjitsu · 2016-11-10T18:02:42Z

src/lib/tooltip/tooltip.ts

+
+  /** Disposes the current tooltip and the overlay it is attached to */
+  private _disposeTooltip(): void {
+    this._overlayRef.dispose();


I am seeming some TypeError: this._overlayRef is null here, I think it originates from here, not 100% sure. Is this a known issue or is there need for one to be opened?

andrewseguin · 2016-11-10T18:11:35Z

I haven't seen this issue, please open an issue with how to reproduce

On Thu, Nov 10, 2016 at 10:02 AM Roland Groza notifications@github.com
wrote:

@rolandjitsu commented on this pull request.

In src/lib/tooltip/tooltip.ts
#1644 (review)
:

}

/** Create the overlay config and position strategy */

private _createOverlay(): void {

let origin = this._getOrigin();

let position = this._getOverlayPosition();

let strategy = this._overlay.position().connectedTo(this._elementRef, origin, position);

let config = new OverlayState();

config.positionStrategy = strategy;

this._overlayRef = this._overlay.create(config);

}

/** Disposes the current tooltip and the overlay it is attached to */

private _disposeTooltip(): void {

this._overlayRef.dispose();

I am seeming some TypeError: this._overlayRef is null here, I think it
originates from here, not 100% sure. Is this a known issue or is there need
for one to be opened?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1644 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AV1nkcKoqIYQy9Duxyctkvi2OUJged10ks5q81xNgaJpZM4Kj73R
.

rolandjitsu · 2016-11-10T18:24:48Z

@andrewseguin I just opened a bunch more issues :)

angular-automatic-lock-bot · 2019-09-06T04:01:13Z

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.}

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Oct 28, 2016

andrewseguin assigned jelbourn Oct 28, 2016

andrewseguin added the pr: needs review label Oct 28, 2016

andrewseguin force-pushed the tooltip-animate branch from fde1c89 to b4d733a Compare October 28, 2016 23:16

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Oct 28, 2016

jelbourn reviewed Oct 29, 2016

View reviewed changes

jelbourn reviewed Oct 31, 2016

View reviewed changes

andrewseguin force-pushed the tooltip-animate branch from b0cb8ed to fb12452 Compare October 31, 2016 21:07

jelbourn reviewed Nov 1, 2016

View reviewed changes

andrewseguin added 11 commits November 1, 2016 10:40

feat(tooltip): init add animations

b72b174

remove f from fdescribe

8d8b6c2

Remove material prefix to animation delay

ace23e3

incorpoate comments

15a7c41

add test for showing after hiding

1cc1f2a

remove fdescribe

aeef90f

fix import wrapping

a825eb4

fix import wrapping

e041442

add test for post-animation

d35831a

remove whitespace

87e0e4d

remove whenStable from fakeAsync test

71f7d42

andrewseguin force-pushed the tooltip-animate branch from 9363443 to 71f7d42 Compare November 1, 2016 17:56

remove fdescribe

6990b4a

jelbourn added pr: lgtm and removed pr: needs review labels Nov 1, 2016

jelbourn added the action: merge The PR is ready for merge by the caretaker label Nov 1, 2016

hansl merged commit 52582f4 into angular:master Nov 1, 2016

jelbourn mentioned this pull request Nov 4, 2016

feat(tooltip): adds animations for tooltip #923

Closed

rolandjitsu reviewed Nov 10, 2016

View reviewed changes

andrewseguin deleted the tooltip-animate branch November 11, 2016 22:59

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tooltip): add tooltip animations #1644

feat(tooltip): add tooltip animations #1644

andrewseguin commented Oct 28, 2016

googlebot commented Oct 28, 2016

googlebot commented Oct 28, 2016

googlebot commented Oct 28, 2016

googlebot commented Oct 28, 2016

jelbourn Oct 29, 2016

jelbourn Oct 29, 2016

jelbourn Oct 29, 2016

jelbourn Oct 29, 2016

jelbourn Oct 29, 2016

jelbourn Oct 29, 2016

jelbourn Oct 29, 2016

andrewseguin commented Oct 31, 2016

jelbourn Oct 31, 2016

jelbourn Oct 31, 2016

andrewseguin commented Oct 31, 2016

andrewseguin commented Oct 31, 2016

jelbourn left a comment

jelbourn Nov 1, 2016

jelbourn Nov 1, 2016

andrewseguin Nov 1, 2016

jelbourn Nov 1, 2016

jelbourn commented Nov 1, 2016

rolandjitsu Nov 10, 2016

jelbourn commented Nov 10, 2016

rolandjitsu commented Nov 10, 2016

andrewseguin commented Nov 10, 2016

rolandjitsu Nov 10, 2016

andrewseguin commented Nov 10, 2016

@rolandjitsu commented on this pull request.

rolandjitsu commented Nov 10, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019


		describe('MdTooltip', () => {
		fdescribe('MdTooltip', () => {

feat(tooltip): add tooltip animations #1644

feat(tooltip): add tooltip animations #1644

Conversation

andrewseguin commented Oct 28, 2016

googlebot commented Oct 28, 2016

googlebot commented Oct 28, 2016

googlebot commented Oct 28, 2016

googlebot commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Oct 31, 2016

andrewseguin commented Oct 31, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Nov 1, 2016

Choose a reason for hiding this comment

jelbourn commented Nov 10, 2016

rolandjitsu commented Nov 10, 2016

andrewseguin commented Nov 10, 2016

Choose a reason for hiding this comment

andrewseguin commented Nov 10, 2016

@rolandjitsu commented on this pull request.

rolandjitsu commented Nov 10, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019