Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(tooltip): support placement="mouse" #527

Closed
wants to merge 1 commit into from
Closed

feat(tooltip): support placement="mouse" #527

wants to merge 1 commit into from

Conversation

lanterndev
Copy link
Contributor

  • uses $position service's new mouse() API
  • works for popover too

* uses $position service's new mouse() API
* works for popover too
@joshdmiller
Copy link
Contributor

Hey @Skivvies - I don't have a problem including something like this, even if Twitter did explicitly choose to exclude it. The code is clean enough for me. Two things, though -

  1. Is this cross-platform? At least WebKit, Firefox, and IE9+?
  2. Can you add some unit tests?

Other than that, looks good to me.

@bekos
Copy link
Contributor

bekos commented Jun 17, 2013

This change unconditionally binds a document mousemove event through position service. Although the operations inside it's handler are not that heavy, it's an unnecessary performance penalty.
Probably you should optimize this, by binding it on mouseenter and unbinding on mouseleave.

@lanterndev
Copy link
Contributor Author

@joshdmiller wrote:

Hey @Skivvies - I don't have a problem including something like this, even if Twitter did explicitly choose to exclude it. The code is clean enough for me. Two things, though -

  1. Is this cross-platform? At least WebKit, Firefox, and IE9+?
  2. Can you add some unit tests?

Other than that, looks good to me.

Awesome! I tested in Chrome, Safari, Firefox, and IE9, and they're all working.

Re unit tests, I actually looked in https://github.com/angular-ui/bootstrap/blob/master/src/tooltip/test/tooltip.spec.js for unit tests for the other placement options ("left", "right", etc.) so I could add one for "mouse", but none of them are currently being tested. I wasn't sure if there was some good reason you're not testing the other options (maybe it's messy to check that the tooltip's position matches expectations), so I didn't add it. Would it make sense to accept this without the mouse-placement-specific unit test, and add tests for all the placement options in a separate PR?

lanterndev added a commit to getlantern/lantern-ui that referenced this pull request Jun 17, 2013
Temporarily use custom built version of angular-ui-bootstrap
while the fix for angular-ui/bootstrap#387 has not yet been released
with additional patch to support tooltip-placement="mouse". Once
angular-ui/bootstrap#527 is merged and 0.4.0 is released, can switch to
including "angular-ui-bootstrap-bower": "~0.4.0" in bower.json.
lanterndev added a commit to getlantern/lantern-ui that referenced this pull request Jun 19, 2013
Temporarily use custom built version of angular-ui-bootstrap
while the fix for angular-ui/bootstrap#387 has not yet been released
with additional patch to support tooltip-placement="mouse". Once
angular-ui/bootstrap#527 is merged and 0.4.0 is released, can switch to
including "angular-ui-bootstrap-bower": "~0.4.0" in bower.json.
@joshdmiller
Copy link
Contributor

Landed as ace7bc6.

@lanterndev
Copy link
Contributor Author

Thanks for merging!

On Sun, Jun 23, 2013 at 1:24 PM, Josh David Miller <notifications@github.com

wrote:

Landed as ace7bc6 ace7bc6
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/527#issuecomment-19877515
.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants