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

fix(tooltip): oversize tooltip, arrow position #1607

Closed
wants to merge 2 commits into from
Closed

fix(tooltip): oversize tooltip, arrow position #1607

wants to merge 2 commits into from

Conversation

khusnetdinov
Copy link

Good day! Create issue after solve the problem. If there is need a something additional, please say - I'll add it.

@pkozlowski-opensource
Copy link
Member

@khusnetdinov thnx for your PR. I see the problem you are trying to fix, but this is not the right approach. Your code assumes certain markup in a template and this is not good.
Moreover the PR is missing tests.

Could you think of a different approach?

@khusnetdinov
Copy link
Author

I tried find the way that not assume markup, I mean not change position for "arrow" element, but the problem is we need to change position any way. Need pass value to css.

May be you can tell me is there better place for it? And could you tell me wiеch exactly test should I write?

About right and left you right, I will fix it.

@ghost ghost assigned bekos and chrisirhc Jan 19, 2014
@chrisirhc
Copy link
Contributor

A better way to do this might be to add a second position value perhaps called arrowPosition and add an ngStyle for the arrow in the popover. This way, there's no reference to the DOM structure (and thus no dependence on how the template should be structured) and the template can be modified by other users and still make use of the arrowPosition.

@khusnetdinov
Copy link
Author

I see the same way solving this problem. Need to pass ng-style="arrowPosition" in arrow class. But a little bit confused how to do it 1 - create directive for arrow or 2 - count it in tooltip provider and pass it to template. There is a provider.

@khusnetdinov
Copy link
Author

If there was just tooltip directive would be simple.

@wesleycho
Copy link
Contributor

Closing this as outdated.

This PR needs to be updated, as we cannot assume DOM structure by default since users may be using custom templates. Feel free to open a new PR that is more current that makes non-breaking changes.

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.

6 participants