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

Arrow out of popover #23846

Closed
alecpl opened this issue Sep 6, 2017 · 17 comments
Closed

Arrow out of popover #23846

alecpl opened this issue Sep 6, 2017 · 17 comments
Labels

Comments

@alecpl
Copy link
Contributor

alecpl commented Sep 6, 2017

Popover arrow can be placed incorrectly when button on the edge of the screen.
arrow-issue
Tested with most recent Bootstrap4 build.

@Johann-S
Copy link
Member

Johann-S commented Sep 6, 2017

Can you make a CodePen of this issue please ? As it mentionned in our contributing guidelines

@alecpl
Copy link
Contributor Author

alecpl commented Sep 6, 2017

@Johann-S I thought it's obvious, but here you go: https://jsbin.com/horagubehe. See that horizontal as well as vertical position is buggy.

@Johann-S
Copy link
Member

Johann-S commented Sep 6, 2017

it seem's has a CSS problem because Popper.js position correctly our arrow but our arrow width is too high 🤔

yes @alecpl but that's easier for us to make a fix with a reproducible bug in live 👍

@Johann-S Johann-S added css and removed js labels Sep 6, 2017
@FezVrasta
Copy link
Contributor

FezVrasta commented Sep 6, 2017

@Johann-S: Popper.js already takes care to include the arrow width in its computations (theorically), make sure the most outer arrow element has the right width set.

edit:

looks like the problem is what I pointed above.
The arrow element should have the same width of the actual space used by the arrow to properly get positioned.
image

edit 2:

If you get rid of the .arrow:before, .arrow:after { left: -7px; }, set .arrow width to 22px and give it a margin-left: 5px; margin-right: 5px you get the proper placement.

Note that the correct width is needed to position the arrow properly, and the margins are used to always ensure some spacing between the arrow and the popover's edge.

image

@Johann-S
Copy link
Member

Johann-S commented Sep 6, 2017

Thank you so much @FezVrasta 👍 if you want to do that changes do not hesitate

@alecpl
Copy link
Contributor Author

alecpl commented Sep 7, 2017

Maybe #23820 should be considered first.

@wojtask9
Copy link
Contributor

wojtask9 commented Sep 7, 2017

Can you check this https://codepen.io/anon/pen/gxNxZw ?

I can make PR to fix this

Edit:
CSS changes with my (not sure if correct) fix for Popper.js

@wojtask9
Copy link
Contributor

wojtask9 commented Sep 7, 2017

I didn't saw #23820
This PR should fix the issue

@wojtask9
Copy link
Contributor

wojtask9 commented Sep 7, 2017

I was wrong. #23820 doesn't fix this issue.
https://codepen.io/anon/pen/ZJdLpW (css compiled with popover changes from #23820)

@Johann-S
Copy link
Member

Johann-S commented Sep 8, 2017

this CodePen : http://codepen.io/anon/pen/gxNxZw seems fine @wojtask9, isn't it ?

@wojtask9
Copy link
Contributor

wojtask9 commented Sep 8, 2017

@Johann-S
Yes.
This pen contains my fix to Popper.js and css popover changes.
I'll make PR when I'll get back home

@jipexu
Copy link
Contributor

jipexu commented Sep 9, 2017

this CodePen : http://codepen.io/anon/pen/gxNxZw seems fine @wojtask9, isn't it ?

the popover bottom left lost his left border ??

@wojtask9
Copy link
Contributor

@jipexu
Something with popper position is wrong. I'm not sure what.

@alecpl
my PR #23936 that should fix your issues

@Johann-S
Copy link
Member

It seems floating-ui/floating-ui#489 fixe this issue, so when @FezVrasta will ship a new release of Popper.js we'll close this issue 👍

@FezVrasta
Copy link
Contributor

Released as v1.12.8

@Johann-S
Copy link
Member

The new release of Popper.js fixe only one part of this issue see : https://codepen.io/Johann-S/pen/qVXEaX

But IMO the first issue is on our CSS here (see the first Popover)

@FezVrasta
Copy link
Contributor

yes, you still need to perform the changes I described in this comment

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

No branches or pull requests

5 participants