Tooltip positioning & placement improvements #3980
Conversation
|
This PR looks pretty solid to me, although another set of eyes couldn't hurt. |
|
I'm not sure if wrapping the call to positionTooltip in $timeout is necessary, have you tried without the $timeouts? |
|
Yeah, the $timeouts were necessary in my testing. It might be possible to set priorities on the directives to remove the need for the $timeouts (so that the position calculation always happened after the text was rendered), but I'm just speculating, I didn't attempt this. |
| if (ttScope.isOpen) { | ||
| $timeout(function () { | ||
| prepPlacement(); | ||
| show()(); |
There was a problem hiding this comment.
Doesn't this mean that if the placement is changed while the tooltip is hidden, it'll show?
There was a problem hiding this comment.
No, it only runs if ttScope.isOpen is true, so only when it's already open.
There was a problem hiding this comment.
Ah right, I missed that check, but there's also when the value is a blank string, see above at line #288: if (!val && ttScope.isOpen ) {
There was a problem hiding this comment.
I think this is a slightly different case that warrants a different conditional. On 288, there's new content to deal with and if the content is blank, it should hide the tooltip. Here there's no new content, just a new placement.
- Only update tooltip position when content changes, instead of every $digest Fixes #3964
|
Thanks @chrisirhc and @RobJacobs for the feedback. I pushed new commits incorporating the suggestions. |
|
The changes look good - if @chrisirhc signs off, this should be good to go. |
|
I built and tested this PR against the existing reproduction cases - this is some good work. I am going to merge it in right now. |
This PR contains two changes:
tooltip-placement/popover-placementto be reflected by the tooltip immediately Tooltip placement does not update dynamically #3978. The test for this is arguably incomplete, it only checks that the tooltip scope is updated, and doesn't check that the tooltip appearance has changed. I don't really know how to test the latter.@chrisirhc It looks like you've done the most work on the tooltips recently, I would love to get your opinion on these changes.