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

fix(tooltip): reposition for placement top #5959

Closed
wants to merge 1 commit into from

Conversation

wesleycho
Copy link
Contributor

  • Reposition the tooltip when the placement option contains top in order to account for resizing of tooltip from right side

I did some research and encountered this in Bootstrap's tooltip service: https://github.com/twbs/bootstrap/blob/master/js/tooltip.js#L282-L284

I replicated this logic, and it can be seen in action here

This correctly fixes #5914

cc @RobJacobs

@RobJacobs
Copy link
Contributor

Nice! LGTM

- Reposition the tooltip when the placement option contains top in order to account for resizing of tooltip from right side
@wesleycho wesleycho force-pushed the fix/tooltip-position branch from 7782804 to 47873cd Compare June 5, 2016 22:13
@wesleycho
Copy link
Contributor Author

Updated the top class check - take another look again when you get the chance.

I want to abstract this logic away into a nice helper method on $uibposition I think, even if it is small. What do you think about that?

@RobJacobs
Copy link
Contributor

I'm all for abstracting logic for re-use. I made a couple of tweaks that I'd like to run by you, see this plunk Biggest changes are:

Moved the height change check to the positionTimeout resolve rather than create another timeout.

When adjusting the css top value, it should be the difference of the initialHeight and currentHeight as the initialHeight of the tooltip is already factored into the css top value.

@wesleycho
Copy link
Contributor Author

Interesting, I like this except for the leaking of data. What are your thoughts on it? Maybe we should merge this and you can file a PR on top of it?

@wesleycho
Copy link
Contributor Author

Going to merge this, we can iterate from here.

@wesleycho wesleycho closed this in 34a1443 Jun 11, 2016
@wesleycho wesleycho deleted the fix/tooltip-position branch June 11, 2016 22:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indeterminant tooltip position with element near screen edge
2 participants