feat(tooltip): Tooltip use MdPanel API #9742

bradrich · 2016-10-03T13:13:50Z

Tooltip has been refactored to use the MdPanel API to position, animate open and closed, and show the tooltip as a result of user interaction.

Breaking Change: Tooltips with not content will no longer show and result in an error.

Fixes #9563

devversion · 2016-10-03T13:34:02Z

src/components/tooltip/tooltip.spec.js

    $material.flushOutstandingAnimations();
  }

  function findTooltip() {
-    return angular.element(document.body).find('md-tooltip');
+    // return angular.element(document.body).find('.md-tooltip');


Remove unnecessary comment.

devversion · 2016-10-03T13:35:46Z

src/components/tooltip/tooltip.spec.js

+      isVisible = true;
+    }
+    $rootScope.$apply(
+      'testModel.isVisible = ' + (isVisible ? 'true' : 'false')


This seems to be a bit ugly.

$rootScope.testModel.isVisible = !!isVisible; $rootScope.$apply();

Alternatively it can also be done as:

$rootScope.$apply(function() { $rootScope.testModel.isVisible = !!isVisible; });

devversion · 2016-10-03T13:36:24Z

src/components/tooltip/tooltip.spec.js

+          expect($rootScope.testModel.isVisible).toBe(false);
+
+          // Clean up document.body.
+          $document[0].body.removeChild(element[0]);


Can be simplified to

element.remove();

devversion · 2016-10-03T13:38:38Z

src/components/tooltip/tooltip.spec.js

+    module(
+      'material.components.tooltip',
+      'material.components.button',
+      'material.components.panel'


Panel should be a dependency on the tooltip itself (so you can remove it here).

This is important for material-tools for example or general the modules we provide (closure or classic)

devversion · 2016-10-03T13:39:29Z

src/components/tooltip/tooltip.scss

@@ -6,66 +6,69 @@ $tooltip-top-margin-lg: rem(1.4) !default;
 $tooltip-top-margin-sm: rem(2.4) !default;
 $tooltip-lr-padding-lg: rem(0.8) !default;
 $tooltip-lr-padding-sm: rem(1.6) !default;
-$tooltip-max-width: rem(3.20) !default;
+$tooltip-max-width: rem(3.2) !default;


I had this comment on the previous PR as well - Why do we use rem here? rem should be for font sizes.

devversion · 2016-10-03T13:42:56Z

src/components/tooltip/tooltip.js

+          // Do not show the tooltip if the text is empty.
+          if (!element[0].textContent.trim()) {
+            throw new Error('Text for the tooltip has not been provided. ' +
+                'Please include text within the mdTooltip element.');


This needs to be marked as a breaking change.

@devversion Not necessarily. In the previous version of the tooltip, the tooltip would not show if there was no text assigned. This only adds the throw.

That's why it should be marked as an breaking change. We intentionally added this behavior (enhancement) on request of users.

See:: #8021

@devversion I didn't realize. What is the necessary action to take for a breaking change? Just want to make sure that I handle it properly.

Normally you just amend the commit and add to the description

Breaking Change: Tooltips no longer show when the content is empty.

devversion · 2016-10-03T13:43:28Z

src/components/tooltip/tooltip.js

+            showTooltip();
+          } else {
+            hideTooltip();
+          }


Can be simplified with a ternary operator.

isVisible ? showTooltip() : hideTooltip();

devversion · 2016-10-03T13:46:24Z

src/components/tooltip/tooltip.js

+            if (
+              autohide ||
+              mouseActive ||
+              $document[0].activeElement !== parent[0]


This does not look very good.

As per the Code guidelines you should start the autohide directly after the if (.

if (autohide || mouseActive ..... // TO LONG $document[0].activeElement !== parent[0]) {

@devversion This is something that I have been struggling with in my own code as well. Where in the Code Guidelines is this reference? I was unable to find it.

@bradrich The specs are not anchor able and not very specific about things, but most of the time the code-sample explains a lot. (We also follow this kind of pattern in all other components - Provide consistency)

@devversion I understand. I have found that the four-space continuation makes things slightly more unreadable compared to the opening of the parenthesis, but this is nothing that I will debate.

@bradrich Yeah, I agree four spaces is a pain.

Helpful advice via Clang format (I ran it on the entire file - here's the result for this method):

function leaveEventHandler() { autohide = scope.hasOwnProperty('mdAutohide') ? scope.mdAutohide : attr.hasOwnProperty('mdAutohide'); if (autohide || mouseActive || $document[0].activeElement !== parent[0]) { // When a show timeout is currently in progress, then we have // to cancel it, otherwise the tooltip will remain showing // without focus or hover. if (showTimeout) { $timeout.cancel(showTimeout); setVisible.queued = false; showTimeout = null; } parent.off(LEAVE_EVENTS, leaveEventHandler); parent.triggerHandler('blur'); setVisible(false); } mouseActive = false; }

So they do the four space continuation on the if statement header, but after that, the inner functionality drops back to a single indentation. @ErinCoughlan, is this formatter something that I might have access to?

Yes, the if statement is a continuation. The method itself isn't, so the 4 indentation doesn't apply.

npm install -g clang-format will install it, but I don't know if we have special flags to format like the style guide.

devversion · 2016-10-03T13:47:37Z

src/components/tooltip/tooltip.js

+        restrict: 'E',
+        // transclude: true,
+        priority: 210, // Before ngAria
+        // template: '<div class="md-content" ng-transclude></div>',


Remove comments (above as well) if those are not valid anymore.

devversion · 2016-10-03T13:48:04Z

src/components/tooltip/tooltip.js

-  .module('material.components.tooltip', [ 'material.core' ])
-  .directive('mdTooltip', MdTooltipDirective)
-  .service('$$mdTooltipRegistry', MdTooltipRegistry);
+    .module('material.components.tooltip', ['material.core'])


As mentioned in another comment, this should depend on the panel module as well.

crisbeto · 2016-10-03T16:47:27Z

src/components/tooltip/tooltip-theme.scss

-  .md-content {
-    background-color: '{{background-700}}';
+.md-tooltip.md-THEME_NAME-theme {
+  color: '{{background-A100}}';


Why were the colors changed here?

@crisbeto That is a good question. I am not sure, but the result was the same. I will have to look into why I changed them.

The colors shouldn't be changed here. It seems you didn't rebase properly here after this PR:

#9207

@clshortfuse The result of the tooltip using the colors that I have specified is the same exact color scheme of the tooltips that are already present in the material demos. Changing them back to the background-700 color doesn't work.

@bradrich I don't know what you mean by "have the same color scheme", but the material design spec says background-700. https://material.google.com/components/tooltips.html#tooltips-tooltips-desktop-

By having these colors changed back, you'd be reintroducing old issues with dark themes and non-gray background palettes.

crisbeto · 2016-10-03T16:49:29Z

src/components/tooltip/tooltip.js

+        var attachTo = angular.element(document.body);
+        var origin = null;
+        var positions = ['top', 'right', 'bottom', 'left'];
+        var position = null;


I think you could combine all of these variables that are initialized to null on one line, e.g. var origin, position, autohide.... Should shorten it down a bit.

crisbeto · 2016-10-03T16:53:11Z

src/components/tooltip/tooltip.js

+          origin = 'md-origin-' + scope.mdDirection;
+
+          // Create the position of the panel based off of the mdDirection.
+          switch (scope.mdDirection) {


It might be a little more concise if we used an enum for the mappings, e.g.:

var POSITIONS = { top: { x: 'CENTER', y: 'ABOVE' } ... }; position = POSITIONS[scope.mdDirection];

crisbeto · 2016-10-03T16:54:53Z

src/components/tooltip/tooltip.js

+        function bindEvents() {
+          // Add a mutationObserver where there is support for it and the need
+          // for it in the form of viable host(parent[0]).
+          if (parent[0] && 'mutationObserver' in $window) {


You've got a typo here. It's supposed to be 'MutationObserver' in $window, instead of 'mutationObserver' in $window.

crisbeto · 2016-10-03T16:59:46Z

src/components/tooltip/tooltip.js

-      }
+          panelConfig = {
+            attachTo: attachTo,
+            template: element.html().trim(),


Why do we need to trim the HTML here?

crisbeto · 2016-10-03T17:02:24Z

src/components/tooltip/tooltip.spec.js

+      isVisible = true;
+    }
+    $rootScope.$apply(
+      'testModel.isVisible = ' + (isVisible ? 'true' : 'false')


Alternatively it can also be done as:

$rootScope.$apply(function() { $rootScope.testModel.isVisible = !!isVisible; });

crisbeto · 2016-10-03T17:07:52Z

src/components/tooltip/tooltip.js

+      function linkFunc(scope, element, attr) {
+        // Set constants.
+        var parent = $mdUtil.getParentWithPointerEvents(element);
+        var attachTo = angular.element(document.body);


From what I'm seeing, attachTo is only being used once in the mdPanel config. It might be more appropriate to inline it.

@crisbeto Good catch. I have removed some others that were also only used in one place.

bradrich · 2016-10-03T18:45:05Z

I have updated everything requested, but GitHub must have changed something. Normally, the inline comments would disappear after the commented line was changed in the next pushed commit. Now, all of your comments are sticking around...

crisbeto · 2016-10-03T21:00:01Z

Overall LGTM on the second look. I'll try to do some manual testing and final review tomorrow.

crisbeto · 2016-10-03T20:58:10Z

src/components/tooltip/tooltip.scss

+$tooltip-height-sm: 32px !default;
+$tooltip-top-margin-lg: 14px !default;
+$tooltip-top-margin-sm: 24px !default;
+$tooltip-lr-padding-lg: 08px !default;


This should be 8px (without the leading zero).

ErinCoughlan

Generally looks good. A lot of the code disappeared, which is cool.

I would love to see Material binary size change as all the components get converted.

ErinCoughlan · 2016-10-03T21:15:51Z

src/components/tooltip/tooltip.js

- * parent. Defaults to 0ms on non-touch devices and 75ms on touch.
- * @param {boolean=} md-autohide If present or provided with a boolean value, the tooltip will hide on mouse leave, regardless of focus
- * @param {string=} md-direction Which direction would you like the tooltip to go?  Supports left, right, top, and bottom.  Defaults to bottom.
+ * @param {expression=} md-visible Boolean bound to whether the tooltip is


ErinCoughlan · 2016-10-03T21:22:54Z

src/components/tooltip/tooltip.js

-              $mdUtil.nextTick(function() {
-                setVisible(false);
-              });
+          if (mutations.some(function(mutation) {


This is a tad confusing in an if statement. Consider making a function (isDisabled or something) to check this.

ErinCoughlan · 2016-10-03T21:27:50Z

src/components/tooltip/tooltip.js

+      var panelAnimation = $mdPanel.newPanelAnimation()
+          .openFrom(parent)
+          .closeTo(parent)
+          .withAnimation({


Would using the built in scale animation with a custom duration work in place of this?

Ideally, I'd want to have to use custom classes rarely, so if there's a way to make panel smarter to cover tooltips, I think it should.

@ErinCoughlan I haven't had a chance to study the new animation work that you and Kristiyan have put together. Can we add a future issue to Github to go back and add the new animation handlers?

@bradrich I had meant the animations from 6 months ago when the component was initially created, but I'm okay with deferring.

ErinCoughlan · 2016-10-03T21:28:51Z

src/components/tooltip/tooltip.js

+        panelClass: 'md-tooltip ' + origin,
+        animation: panelAnimation,
+        position: panelPosition,
+        zIndex: 100,


At some point, consider making this customizable for uses of the tooltip. It's a problem my team has run into several times.

I agree. I am adding this now. This actually came up in a demo with Topher.

ErinCoughlan · 2016-10-03T21:29:41Z

src/components/tooltip/tooltip.js

-      if ( !scope.visible ) return;
+      var panelConfig = {
+        attachTo: attachTo,
+        template: element.html(),


Should we implement the content element in panel to make this easier?

@ErinCoughlan In this case, I am grabbing the internal content of the <md-tooltip></md-tooltip> element and transposing it into the panel element. I think that it would complicate things to introduce the content element when all you want is the tooltip text here.

ErinCoughlan · 2016-10-03T21:30:32Z

src/components/tooltip/tooltip.js

-   * @param  {Event} event Event object passed in by the browser.
+   * Global event handler that dispatches the registered handlers in the
+   * service.
+   * @param {Event} event Event object passed in by the browser


ErinCoughlan · 2016-10-03T21:35:32Z

src/components/tooltip/tooltip.js

-   * @param  {Boolean} useCapture Whether to use event capturing.
+   * @param {string} type Type of event to be registered.
+   * @param {!Function} handler Event handler.
+   * @param {boolean} useCapture Whether to use event capturing.
   */
  function register(type, handler, useCapture) {
    var array = listeners[type] = listeners[type] || [];


This is a horrible variable name. Variable names should be descriptive of what it contains, not it's type. (I know if was here before, but consider changing to handlers or listenersForType.)

ErinCoughlan · 2016-10-03T21:36:39Z

src/components/tooltip/tooltip.scss

  pointer-events: none;
  border-radius: 4px;
+  overflow: hidden;
+


What's with all the new lines between "sections"?

I went ahead and removed 90% of them.

crisbeto · 2016-10-04T16:00:06Z

src/components/tooltip/tooltip.js

-          $$mdTooltipRegistry.deregister('blur', windowBlurHandler);
-          $$mdTooltipRegistry.deregister('resize', debouncedOnResize);
+      function isDisabledMutation() {
+        mutations.some(function(mutation) {


@bradrich This throws an error. Seems like you forgot to pass in the mutations.

crisbeto · 2016-10-04T16:01:32Z

src/components/tooltip/tooltip.js


-      updateContentOrigin();
-      positionTooltip();
+      panelRef = $mdPanel.create(panelConfig);


This seems very inefficient, because it creates a new panel ref on every open. I think we should create one only after the first open and then re-use it.

You are correct about the inefficiency. I forgot to add the ID parameter to the panel configuration. That will make it only create the panel once for each of the tooltips.

crisbeto · 2016-10-04T16:05:29Z

@bradrich it seems like the margins on the tooltip has changed. Here's a side-by-side (left is what the current tooltip looks like):

bradrich · 2016-10-04T18:04:54Z

@crisbeto You are correct about the new spacing. According to the Material Guidelines, there is supposed to be 14 pixels between the element and the tooltip on a desktop. 24 pixels if you are on a mobile device. I have positioned the tooltip according to those requirements.

bradrich · 2016-10-06T19:05:11Z

@crisbeto @ErinCoughlan Please take a look at the final revisions and provide an LGTM if everything is GTG. Thank you.

ThomasBurleson · 2016-10-09T12:08:44Z

@crisbeto , @ErinCoughlan - ping request for lgtm.

crisbeto

Did another review and code-wise it LGTM, apart from the minor comment about the id generation, but that isn't something blocking.

Functionality-wise, I noticed that doing a lot of enter/leave actions causes unexpected behavior. E.g. in this case once my mouse stops on the trigger, the tooltip is supposed to stay visible:

For reference, here's what happens with the current implementation:

I don't know whether this isn't an mdPanel issue where the animations get stacked up.

crisbeto · 2016-10-09T12:49:34Z

src/components/tooltip/tooltip.js

+
+      // Convert the content to camelCase to create the tooltip panel id.
+      // Note: This will also remove any non-alpha characters.
+      var id = content


@bradrich Why do we have to use the content to generate an id for the panel? This is seems like a lot of trouble for something that could easily be done with "tooltip-" + $mdUtil.nextUid(), in addition to potentially having multiple tooltips with the same id, if they have the same content.

@crisbeto This is a very good point. I have made this alteration.

@bradrich - please debounce the showing/hiding of the tooltip.

ErinCoughlan

LGTM/Approval after the comments are addressed.

ErinCoughlan · 2016-10-10T16:34:00Z

src/components/tooltip/tooltip.js

-      visible: '=?mdVisible',
-      autohide: '=?mdAutohide',
-      direction: '@?mdDirection'    // only expect raw or interpolated string value; not expression
+      mdZIndex: '=?mdZIndex',


optional: since you are no longer renaming these, you don't need to write the attribute name again:

mdZIndex: '=?', mdDelay: '=?', ...

ErinCoughlan · 2016-10-10T16:36:35Z

src/components/tooltip/tooltip.js

+  var TOOLTIP_DEFAULT_SHOW_DELAY = 0;
+  var TOOLTIP_DEFAULT_DIRECTION = 'bottom';
+  var TOOLTIP_DIRECTIONS = {
+    top: { x: 'CENTER', y: 'ABOVE' },


This is be more robust if you use the enum defined in mdPanel instead of writing them here. If, for instance, I decide that the text shouldn't be capitalized and change it, this entire component will break and it shouldn't.

ErinCoughlan · 2016-10-10T16:42:18Z

src/components/tooltip/tooltip.spec.js

+          expect($rootScope.testModel.isVisible).toBe(false);
+
+          // Clean up document.body.
+          element.remove();


Do you want to do something like this in the afterEach to be extra safe from someone forgetting to clean up?

ThomasBurleson · 2016-10-11T17:23:31Z

@bradrich - ping regard last requests from @ErinCoughlan .

bradrich · 2016-10-11T18:09:02Z

@ThomasBurleson All clear!

bradrich · 2016-10-17T17:16:08Z

@ThomasBurleson This one is all clear and good to go. Conflicts have been resolved.

clshortfuse · 2016-10-19T15:12:10Z

src/components/tooltip/tooltip.scss

  &.md-show {
    transition: $swift-ease-out;
-    pointer-events: auto;
+    transform: scale(1);
+    opacity: 1;


Opacity should be 0.9 (90%) as in master

Tooltip has been refactored to use the MdPanel API to position, animate open and closed, and show the tooltip as a result of user interaction. Breaking Change: Tooltips with no content will no longer show and result in an error. Fixes angular#9563

bradrich · 2016-11-02T13:07:21Z

@ThomasBurleson Checking on the status of this PR.

bradrich added the - Use $mdPanel label Oct 3, 2016

bradrich added this to the - Backlog milestone Oct 3, 2016

bradrich assigned ErinCoughlan and crisbeto Oct 3, 2016

devversion reviewed Oct 3, 2016

View reviewed changes

crisbeto suggested changes Oct 3, 2016

View reviewed changes

bradrich force-pushed the feat/panel-tooltip branch 2 times, most recently from c9f476a to 608bb4e Compare October 3, 2016 18:41

crisbeto suggested changes Oct 3, 2016

View reviewed changes

ErinCoughlan reviewed Oct 3, 2016

View reviewed changes

bradrich force-pushed the feat/panel-tooltip branch from 608bb4e to 21678ad Compare October 4, 2016 14:54

crisbeto suggested changes Oct 4, 2016

View reviewed changes

bradrich force-pushed the feat/panel-tooltip branch from 21678ad to 238b5a1 Compare October 6, 2016 19:04

devversion mentioned this pull request Oct 7, 2016

chore: add karma chrome launcher #9778

Merged

ThomasBurleson modified the milestones: 1.1.2, - Backlog Oct 9, 2016

ThomasBurleson added type: enhancement in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Oct 9, 2016

crisbeto approved these changes Oct 9, 2016

View reviewed changes

bradrich force-pushed the feat/panel-tooltip branch from 238b5a1 to 9ee73b1 Compare October 10, 2016 13:01

ErinCoughlan approved these changes Oct 10, 2016

View reviewed changes

bradrich force-pushed the feat/panel-tooltip branch 2 times, most recently from 1f1aa13 to 2350930 Compare October 10, 2016 18:40

bradrich force-pushed the feat/panel-tooltip branch from 2350930 to 4ca0d64 Compare October 17, 2016 17:16

bradrich removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Oct 17, 2016

ThomasBurleson added the needs: presubmit label Oct 17, 2016

clshortfuse reviewed Oct 19, 2016

View reviewed changes

bradrich force-pushed the feat/panel-tooltip branch from 4ca0d64 to 1bc01da Compare October 19, 2016 17:01

feat(tooltip): Tooltip use MdPanel API

ffa32b1

Tooltip has been refactored to use the MdPanel API to position, animate open and closed, and show the tooltip as a result of user interaction. Breaking Change: Tooltips with no content will no longer show and result in an error. Fixes angular#9563

bradrich force-pushed the feat/panel-tooltip branch from 1bc01da to ffa32b1 Compare October 19, 2016 17:21

kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Nov 15, 2016

kara merged commit 6d06188 into angular:master Nov 15, 2016

angular locked as resolved and limited conversation to collaborators Mar 7, 2020

Splaktar unassigned ErinCoughlan Mar 7, 2020

feat(tooltip): Tooltip use MdPanel API #9742

feat(tooltip): Tooltip use MdPanel API #9742

Conversation

bradrich commented Oct 3, 2016 • edited

devversion Oct 3, 2016 • edited

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

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

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

ErinCoughlan Oct 3, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Oct 3, 2016 • edited

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

bradrich commented Oct 3, 2016

crisbeto commented Oct 3, 2016

crisbeto Oct 3, 2016 • edited

Choose a reason for hiding this comment

ErinCoughlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErinCoughlan Oct 3, 2016 • edited

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

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

crisbeto commented Oct 4, 2016

bradrich commented Oct 4, 2016

bradrich commented Oct 6, 2016

ThomasBurleson commented Oct 9, 2016

crisbeto left a comment

Choose a reason for hiding this comment

crisbeto Oct 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErinCoughlan 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

ThomasBurleson commented Oct 11, 2016

bradrich commented Oct 3, 2016 •

edited

devversion Oct 3, 2016 •

edited

ErinCoughlan Oct 3, 2016 •

edited

crisbeto Oct 3, 2016 •

edited

crisbeto Oct 3, 2016 •

edited

ErinCoughlan Oct 3, 2016 •

edited

crisbeto Oct 9, 2016 •

edited