feat(tooltip): now can take the option for the close delay timeout #3576

tmcgee123 · 2015-04-21T20:14:56Z

test(tooltip): testing that the close delay works as expected

This is a change to add control over the delay to remove the tooltip from the view. This solution came from the need to delay the closing of the tooltip without overriding bootstrap classes, manipulation of the DOM , or need to create custom triggers that depend on the usage of $timeout.

The use-case for this was:

User hovers over the link for the popover/tooltip
The popover/tooltip shows long enough for the user to move the cursor off of the element that triggers the popover and access any link inside of the popover/tooltip content

dmitriz · 2015-04-25T05:21:43Z

src/tooltip/test/tooltip.spec.js

+        'tooltip-popup-close-delay="{{delay}}">Selector Text</span>'
+      ))(scope);
+      elmScope = elm.scope();
+      tooltipScope = elmScope.$$childTail;


Isn't using Angular's internal method $$childTail creating a potential for future bugs, should its implementation change in the future?

If I understand it correctly, this is to access child scope, which we should try to avoid to.
Here it is suggested instead to

move the model up into the parent...
http://stackoverflow.com/a/14006165/1614973

I've used the same setup as the test for the delay of the opening of the tooltip in the file, if you were to look at the rest of the spec it is doing the same. In a test, this is standard practice to access the directive's scope in order to check a value.

This is just a concern that came up, I know it works for now, but might be helpful for future reference. There might be a better way without depending so deeply on Angular's mechanics.
I didn't want to add to already huge issue queue, so put it here.

tmcgee123 · 2015-05-04T18:40:22Z

I was able to successfully change each .trigger to .triggerHandler in this file. Also, I added more assertions for checking whether or not the element is visible and not just "dirty" checking the isOpen scope variable as @dmitriz has suggested for both. The only comment I was unable to address was the one pertaining to $$childTail as the isOpen variable is not accessible publicly and is truly "isolate" to this directive, but is set as a TODO on line 142 of tooltip.js where it states "// TODO add ability to start tooltip opened".

wesleycho · 2015-07-20T12:20:29Z

src/tooltip/test/tooltip.spec.js

@@ -33,7 +33,7 @@ describe('tooltip', function() {
  }));

  it('should open on mouseenter', inject(function() {
-    elm.trigger( 'mouseenter' );


Any particular reason for all these changes from trigger to triggerHandler?

@dmitriz noted above that trigger is from the jQuery dependency: "It is correct that .trigger is currently used but it is from jQuery. The present tests have been written a while ago, but more recently the Project Philosophy sets the goal to get away from the jQuery dependency. So for new PRs, it might be a worth contribution towards the new Philosophy to support this trend." Please check the outdated comments above from the changes I made.

I would undo those changes - they can be tackled in a separate PR, but they pollute the feature.

This has been properly reverted

wesleycho · 2015-07-20T12:24:40Z

I like this change for it to be more configurable, but this PR seems polluted with some non-relevant changes.

Also for verification, I notice that the account that made the commits is different from the one filing the PR - I assume they are both yours?

tmcgee123 · 2015-07-20T15:14:59Z

@wesleycho can you elaborate on the "pollution" you mentioned above? I'm not sure what you mean by the account that made the commits is different, it's just showing my new avatar and updated profile. I'm the only person making these changes if that's what you are asking :D

wesleycho · 2015-08-05T05:41:59Z

package.json

@@ -1,7 +1,7 @@
 {
  "author": "https://github.com/angular-ui/bootstrap/graphs/contributors",
  "name": "angular-ui-bootstrap",
-  "version": "0.13.2-SNAPSHOT",
+  "version": "0.13.2-A1.4.3",


Undo this change.

wesleycho · 2015-08-05T05:48:58Z

I have considered opening code style change PRs and altering the build to enforce it, but it has the downside of instantly merge conflicting almost all PRs - an incremental improvement in style would help. If it is not made, it would likely be made on merging of the PR, which puts more burden on maintainers (it was already painful enough cutting issues down from over 500 to under 330, and open PRs from over 200 to almost 100 the past couple of months).

tmcgee123 · 2015-08-10T18:55:20Z

@wesleycho I've made all of the recommended changes that you've left comments on. Please let me know if there are any other changes! Thanks for the help!

wesleycho · 2015-08-10T19:03:06Z

src/tooltip/test/tooltip.spec.js

@@ -853,9 +938,9 @@ describe( '$tooltipProvider', function() {
        elm.trigger('fakeTrigger');
        expect( tooltipScope.isOpen ).toBeTruthy();
        elm.trigger('fakeTrigger');
+        $timeout.flush();


Is this change necessary?

Yes, the test was failing on my branch since there is a timeout for closing the popup.

wesleycho · 2015-08-10T19:05:06Z

The history needs to be cleaned up - there should not be a merge commit, and it should be squashed into one.

tmcgee123 · 2015-08-10T20:48:07Z

@wesleycho I was able to successfully rebase and squash my commits. That was a first for me, barely made it through whew.

tmcgee123 · 2015-09-11T18:26:07Z

@wesleycho I've updated this again to resolve the merge conflicts, any chance on this getting merged soon? It'd be nice to be able to point to the main repository rather than pointing to my fork. Thanks!!

wesleycho · 2015-10-03T07:23:18Z

I apologize for having to ask this again, but can you rebase off of master & adjust the feature one more time? Some major fixes landed with regards to tooltip positioning & $digest errors.

I intend on having this land for 0.14, so the sooner, the better :) .

tmcgee123 · 2015-10-05T16:21:41Z

Okay Mr. @wesleycho , I went ahead and cleared my changes and re-applied them, as I was stuck in rebase misery :D still need to learn how to do that properly. Cheers!

wesleycho · 2015-10-05T16:32:42Z

src/tooltip/docs/readme.md

@@ -20,6 +20,7 @@ will display:
 - `tooltip-animation`: Should it fade in and out? Defaults to "true".
 - `tooltip-popup-delay`: For how long should the user have to have the mouse
  over the element before the tooltip shows (in milliseconds)? Defaults to 0.
+- `tooltip--close-popup-delay`: For how long should the tooltip remained open?


wesleycho · 2015-10-05T16:33:53Z

LGTM - I'll merge this soon with the fixes applied.

tmcgee123 · 2015-10-05T18:08:20Z

Thanks!!!

ArielAleksandrus · 2015-10-10T03:39:02Z

Does it work with popover? I'm not being able to use it with neither popover nor tooltip. Can I pass a literal value or it should be an angular variable? Is the time in milliseconds?

wesleycho · 2015-10-10T07:40:39Z

It should work with both - this seems to not work as expected though.

tmcgee123 · 2015-10-12T15:51:30Z

It looks like in the midst of the many changes that were conflicting with my branch, the functionality stopped working.

For some reason, even though the $timeout is called in the hide function on line 269 in tooltip.js, it is not properly timing out. The ttScope.popupCloseDelay variable is being properly set to the passed in delay, so there must be something cancelling the timeout or something else that I am not aware of is going on.

if (ttScope.animation) {
  if (!transitionTimeout) {
    transitionTimeout = $timeout(removeTooltip, ttScope.popupCloseDelay); // here is where $timeout is not working for some reason
  }
} else {
  removeTooltip();
}

I'm still debugging, surely the fix will be simple. Any thoughts, points of interest, or information would be much appreciated!

wesleycho · 2015-10-12T15:59:31Z

I noticed this too this past weekend, but I didn't have time to debug since I was out for a large part of it - if you don't find out what happened, I'll probably take a look again sometime later this week.

tmcgee123 · 2015-10-12T16:40:32Z

Ahh, it's because of the $evalAsync that is being run before we wait for the animation to completely remove it from the DOM. Since it sets the visibility to false, it is still present in the DOM, but not visible.

I was able to get the delay to work by changing this:

ttScope.$evalAsync(function() {
  ttScope.isOpen = false;
  if (isOpenParse && angular.isFunction(isOpenParse.assign)) {
    isOpenParse.assign(ttScope.origScope, ttScope.isOpen);
  }
});

to this (I'm not sure if this is correct since we using both $timeout and $evalAsync):

$timeout(function () {
  ttScope.$evalAsync(function() {
    ttScope.isOpen = false;
    if (isOpenParse && angular.isFunction(isOpenParse.assign)) {
      isOpenParse.assign(ttScope.origScope, ttScope.isOpen);
    }
  });
}, ttScope.popupCloseDelay);

Also, there is a potential for a slight refactor, since there is shared functionality inside of the callbacks for each $evalAsync:

...
ttScope.isOpen = false;
if (isOpenParse && angular.isFunction(isOpenParse.assign)) {
  isOpenParse.assign(ttScope.origScope, ttScope.isOpen);
}
...

Maybe we can make a function called assignIsOpenToScope (need better name for this)?

function assignIsOpenToScope(isOpen) {
  ttScope.isOpen = isOpen;
  if (isOpenParse && angular.isFunction(isOpenParse.assign)) {
    isOpenParse.assign(ttScope.origScope, ttScope.isOpen);
  }
}

RobJacobs · 2015-10-12T18:16:21Z

There is already a timeout for when the removeTooltip() is getting called, I would rather see the hideTooltipBind() restructured to work like the showTooltipBind(). I'll put together a PR for review shortly.

tmcgee123 · 2015-10-12T18:31:00Z

I agree, I think it's rather silly that hideTooltipBind simply just calls hide. We can probably just use one or the other.

RobJacobs · 2015-10-12T20:13:26Z

@tmcgee123 PR #4597 has been created to address this issue.

karianna added component: tooltip needs: review labels Apr 21, 2015

karianna added this to the 0.13.x milestone Apr 21, 2015

tmcgee123 mentioned this pull request Apr 23, 2015

Popover to remain open when hovering over it #3515

Closed

dmitriz reviewed Apr 25, 2015
View reviewed changes

wesleycho reviewed Jul 20, 2015
View reviewed changes

wesleycho added needs: work and removed needs: review labels Jul 20, 2015

wesleycho reviewed Aug 5, 2015
View reviewed changes

wesleycho reviewed Aug 10, 2015
View reviewed changes

wesleycho added needs: rebasing needs: squash labels Aug 10, 2015

wesleycho added needs: review and removed needs: rebasing needs: squash needs: work labels Aug 13, 2015

wesleycho modified the milestones: 0.13.x, 0.14.0 (Bootstrap 3.3) Sep 3, 2015

wesleycho added needs: rebasing and removed needs: review labels Oct 3, 2015

tmcgee123 closed this Oct 5, 2015

feat(tooltip): add option for delaying the closing of the tooltip

68bb423

tmcgee123 reopened this Oct 5, 2015

wesleycho added needs: review and removed needs: rebasing labels Oct 5, 2015

wesleycho reviewed Oct 5, 2015
View reviewed changes

wesleycho removed the needs: review label Oct 5, 2015

wesleycho closed this in 5f7051b Oct 5, 2015

RobJacobs mentioned this pull request Oct 7, 2015

Use $animate for tooltip animation #3531

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tooltip): now can take the option for the close delay timeout #3576

feat(tooltip): now can take the option for the close delay timeout #3576

tmcgee123 commented Apr 21, 2015

dmitriz Apr 25, 2015

tmcgee123 Apr 25, 2015

dmitriz Apr 26, 2015

tmcgee123 commented May 4, 2015

wesleycho Jul 20, 2015

tmcgee123 Jul 20, 2015

wesleycho Jul 20, 2015

tmcgee123 Jul 20, 2015

wesleycho commented Jul 20, 2015

tmcgee123 commented Jul 20, 2015

wesleycho Aug 5, 2015

wesleycho commented Aug 5, 2015

tmcgee123 commented Aug 10, 2015

wesleycho Aug 10, 2015

tmcgee123 Aug 10, 2015

wesleycho commented Aug 10, 2015

tmcgee123 commented Aug 10, 2015

tmcgee123 commented Sep 11, 2015

wesleycho commented Oct 3, 2015

tmcgee123 commented Oct 5, 2015

wesleycho Oct 5, 2015

wesleycho commented Oct 5, 2015

tmcgee123 commented Oct 5, 2015

ArielAleksandrus commented Oct 10, 2015

wesleycho commented Oct 10, 2015

tmcgee123 commented Oct 12, 2015

wesleycho commented Oct 12, 2015

tmcgee123 commented Oct 12, 2015

RobJacobs commented Oct 12, 2015

tmcgee123 commented Oct 12, 2015

RobJacobs commented Oct 12, 2015

feat(tooltip): now can take the option for the close delay timeout #3576

feat(tooltip): now can take the option for the close delay timeout #3576

Conversation

tmcgee123 commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmcgee123 commented May 4, 2015

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

wesleycho commented Jul 20, 2015

tmcgee123 commented Jul 20, 2015

Choose a reason for hiding this comment

wesleycho commented Aug 5, 2015

tmcgee123 commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleycho commented Aug 10, 2015

tmcgee123 commented Aug 10, 2015

tmcgee123 commented Sep 11, 2015

wesleycho commented Oct 3, 2015

tmcgee123 commented Oct 5, 2015

Choose a reason for hiding this comment

wesleycho commented Oct 5, 2015

tmcgee123 commented Oct 5, 2015

ArielAleksandrus commented Oct 10, 2015

wesleycho commented Oct 10, 2015

tmcgee123 commented Oct 12, 2015

wesleycho commented Oct 12, 2015

tmcgee123 commented Oct 12, 2015

RobJacobs commented Oct 12, 2015

tmcgee123 commented Oct 12, 2015

RobJacobs commented Oct 12, 2015