Skip to content

fix popover arrow computations#23110

Closed
wojtask9 wants to merge 5 commits intotwbs:v4-devfrom
wojtask9:poppover_arrows
Closed

fix popover arrow computations#23110
wojtask9 wants to merge 5 commits intotwbs:v4-devfrom
wojtask9:poppover_arrows

Conversation

@wojtask9
Copy link
Copy Markdown
Contributor

@wojtask9 wojtask9 commented Jul 17, 2017

Current border of popover arrow can be only 1px.

This PR takes into account popover border too.

You can play with arrow border here: http://www.cssarrowplease.com/

Also this PR fixes white arrow background with popover-header

before:
image

after:
image

Comment thread scss/_popover.scss

.arrow::before,
.arrow::after {
top: 100%;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use any CSS for the position of our arrow, the arrow position is managed by Popper.js not our CSS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

popper.js manages .arrow not .arrow:before and .arrow:after.

So it's safe to position these pseudo-elements.

Copy link
Copy Markdown
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove any css rules which manage the arrow position please

@Johann-S Johann-S dismissed their stale review July 18, 2017 06:06

You're right @wojtask9 maybe you can add a Codepen to show your work

@wojtask9
Copy link
Copy Markdown
Contributor Author

default border-width
https://codepen.io/anon/pen/ZyNxeN

border width 3px;
https://codepen.io/anon/pen/jwozmQ

Sadly I see bug with popover with header.
If popper.js starts manage .arrow then positioning is not added to popover-header and my fix isn't working :/ (see popover on right button)

@Johann-S
Copy link
Copy Markdown
Member

You can change that line : https://github.com/twbs/bootstrap/blob/v4-dev/js/src/tooltip.js#L103
by : .arrow, .arrow::before, .arrow::after
It's the selector used by Popper.js to manage arrow position

@wojtask9
Copy link
Copy Markdown
Contributor Author

But then additional html markup is required (not hard because only template must be modified)
Without template modifications I don't know how to manipulate popover-header:before pseudo-element.

Comment thread scss/_popover.scss Outdated
width: 20px;
margin-left: -10px;

.popover-header:not(:empty) + .arrow:after {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Begin pseudo elements with double colons: ::

@wojtask9
Copy link
Copy Markdown
Contributor Author

wojtask9 commented Jul 19, 2017

I've changed .arrow order in popover template. Now we use adjacent sibling selector to detect if .popover-header is visible or not.
Everything should works correct.

updated codepen:
https://codepen.io/anon/pen/gRNxMd

@wojtask9
Copy link
Copy Markdown
Contributor Author

also artifacts on on certain zoom level could be solved.
Instead of border on .popover use box-shadow with comma.
Not sure if bootstrap care about browser zoom.

@Johann-S
Copy link
Copy Markdown
Member

No we don't handle browser zooming

@NielsHolt
Copy link
Copy Markdown

Hi
I think #23820 solve this issue without changing the template and also adding the use of rem in dimension the border and arrow

@wojtask9
Copy link
Copy Markdown
Contributor Author

@NielsHolt
According to arrow scss computations your fix is better. I'll close this PR.
But without changing template you are not able to correctly position .popover-header::before.
With static example it looks OK but when arrow is not centered then you will observe artifacts

@wojtask9
Copy link
Copy Markdown
Contributor Author

#23936 better fix

@wojtask9 wojtask9 closed this Sep 13, 2017
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.

5 participants