Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tooltip/popover arrow size and position #25062

Merged
merged 1 commit into from
Dec 27, 2017
Merged

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Dec 23, 2017

This overhauls the tooltip and popover arrows implementation, fixing a few problems:

1. arrows were not properly aligned to the trigger.

Fixes #25045 and fixes #23793

2. the .arrow element did not match the size and position of the actual visible arrow.

Before:
image

After:
image

Same for tooltips. This possibly also fixes #23846, see #23846 (comment).

3. Fixed $popover-arrow-height / $tooltip-arrow-height

These Sass variables did not have any effect on the visual appearance. They were just used for the (wrong, see above) sizing of the .arrow element, which has absolutely no visible effect. Only $...-arrow-width had a visual effect, but it always determined implicitly both the width and height (height = width/2, so a 45° angle).

Also the value for width was wrongly used, AFAICT. Setting it to 10px would lead to an arrow 20px wide. My understanding of these dimensions is that $popover-arrow-width should specify the width of the .arrow (for top or bottom placement), as can be seen on the second screenshot above.

This has been fixed here. But for anyone who has customized the arrows with these variables, this would be a somewhat breaking change (the width will be the half of it as it has been before). But still I think this has to be done, as this restores that the assigned values now exactly match the visual dimensions.

@mdo mdo mentioned this pull request Dec 27, 2017
@mdo mdo merged commit 9600ab1 into twbs:v4-dev Dec 27, 2017
@Johann-S
Copy link
Member

Thank you so much @simonihmig 👍

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

Successfully merging this pull request may close these issues.

3 participants