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

feat(tooltip): Update tooltip placement after height changes #1110

Closed
wants to merge 1 commit into from
Closed

feat(tooltip): Update tooltip placement after height changes #1110

wants to merge 1 commit into from

Conversation

calvinfroedge
Copy link

The tooltip was not correctly updating it's placement (see issue #1109) after dom elements were inserted above it because it was only checking placement when the tooltip was original placed. I can think of lots of scenarios where this would cause issues (rotating variable height content above, ajax results displayed above, form validation helpers above).

I added a listener using setInterval and ran tests to make sure all still pass. Issue is corrected.

I am happy to write test but how do I know in my test if the positioning is changing when dom elements change? How can I add dom elements and check style properties?

@calvinfroedge
Copy link
Author

Eh, sorry about the tabs / spaces issue

@pkozlowski-opensource
Copy link
Member

@calvinfroedge I would like to know if your use-case is the following one: tooltip / popover gets opened, and while it stays opened (probably easier to spot with popovers) something changes on the page (DOM) making a tooltip / popover "badly" positioned? If so there is an issue opened for this already: #96

Now, regarding your patch, I'm not super-fan of constantly (setInterval) quering / calculating dimensions as this is rather expensive operation. So, as the very minimum it should be opt-in configuration option and not the default option. So in the current form this PR can't be merged (not to mention that it breaks the build).

I would really like to know more about your use-case to decide upon the best curse of action here.

@pkozlowski-opensource
Copy link
Member

@calvinfroedge oh, sorry, only now I saw your use-case in the video attached to the issue #1109, your use-case is perfectly clear and something I've expected.

I've just tested the original Bootstrap and it has the same "bug". #96, while easy to fix, won't address your issue and it is true that I don't see any other magical solution than periodically re-calculate positioning :-( But this is really "expensive" so we should do with caution... There must be a better solution out there :-)

@calvinfroedge
Copy link
Author

Is there another method of "watching" the position? I tried to pick the least evil option. The interval is only in defined while the popover / tooltip is actually open.

Apparently my change broke the build because of tabs vs spaces issue. Unit tests did not complain locally so I didn't know until I pushed.

@chrisirhc
Copy link
Contributor

Hi @calvinfroedge, thank you for your contribution. I believe #1415 supersedes this pull request. Due to recent changes (tooltip's scope only exists when its visible), adding a watcher that updates the position isn't so evil, heh.

@chrisirhc chrisirhc closed this Jan 16, 2014
codedogfish pushed a commit to codedogfish/angular-ui-bootstrap that referenced this pull request Sep 15, 2015
Limit the maximum number of selections allowed in multiple mode
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