Skip to content

fix popover arrow computations and .arrow position#23936

Closed
wojtask9 wants to merge 1 commit intotwbs:v4-devfrom
wojtask9:popover_array_2
Closed

fix popover arrow computations and .arrow position#23936
wojtask9 wants to merge 1 commit intotwbs:v4-devfrom
wojtask9:popover_array_2

Conversation

@wojtask9
Copy link
Contributor

@wojtask9 wojtask9 commented Sep 13, 2017 •
edited
Loading

This fix is based on #23820 but it also fixes 'corner cases' from #23846

According to comment #23846 (comment) "The arrow element should have the same width of the actual space used by the arrow to properly get positioned" and "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".

Also this PR fixes white arrow background with popover-header.
Note that 'old' solution is not valid anymore because .arrow position is managed by popper.js so it must be relative to .arrow

before:

after:

Notes:

  1. This fix requires If popup contains 'border' then arrow is incorectly positioned floating-ui/floating-ui#417 if we want .popover border larger than 1px.
  2. converting to rem is very easy.
  3. calc() can be removed if border and width use the same units
  4. thanks to @NielsHolt and his PR (Popup and tooltip: dimensions in rem and bug fix of arrow placement #23820) it should work also with browser zoom without any issues
  5. var $popover-arrow-width currently means "whole" arrow width (before it was only half). The same with $popover-arrow-height.
  6. $popover-arrow-margin is used to indicate margin between popover edge. We cannot set this to 0 because by default .popover use border-radius.

@wojtask9 wojtask9 changed the title fix popover arrow computations and .arrow position with fix popover arrow computations and .arrow position Sep 13, 2017
This was referenced Sep 13, 2017
Closed
Closed
Closed
vsn4ik
vsn4ik reviewed Sep 13, 2017
View reviewed changes
scss/_popover.scss
// Arrows
//
// .arrow is outer, .arrow::after is inner
// .arrow:before is outer, .arrow::after is inner
Copy link
Contributor

@vsn4ik vsn4ik Sep 13, 2017

Choose a reason for hiding this comment

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

CSS2 -> CSS3

@NielsHolt
Copy link

NielsHolt commented Sep 19, 2017

Hi @wojtask9 and @vsn4ik
I am not quite sure why you think that #23820 doesn't solve the corner problem
But there are some issues with #23936 when the border-width <> 1px

I have created tree demo to illustrate:
1: Bootstrap 4-dev with $border-width: 10px:
2: This PR with $border-width: 1rem
3: #23820 with $border-width: 1rem

Could you please have a look at them and let me hear what you think :-)

@wojtask9
Copy link
Contributor Author

wojtask9 commented Sep 20, 2017

@NielsHolt
Have a look at this codepen https://codepen.io/anon/pen/wrGEWo (css compiled from #23820) with border 1px.

From popper.js docs about .arrow: "It will read the outer size of the arrowElement node to detect how many pixels of conjuction are needed"

In your PR you didn't take this into account and .arrow::before is bigger than .arrow.
It works with border 1rem but not necessary with other sizes.

Also popper.js takes arrow margins into account to compute distance between popover edge and arrow.
For example if we don't have border radius on popover then margin can be '0'.
But with rounded popover you cannot set margin to '0' because then you will see that .arrow is disconnected from popover.

In my PR (as you shown) there are issues but I believe these are issues in popper.js.

@NielsHolt
Copy link

NielsHolt commented Oct 2, 2017

Hi @wojtask9
My PR #23820 was closed by @mdo so I will leave it to you to fix it :-)

@Johann-S Johann-S requested a review from andresgalante October 6, 2017 08:05
@mdo
Copy link
Member

mdo commented Oct 19, 2017

Passing on this given recent changes to popovers using rems from another PR. I'd be happy to see another PR that addresses scaling the arrow borders with the popover's border width, but I don't see any advantage in the rest of these changes here. Thanks though!

@mdo mdo closed this Oct 19, 2017
@wojtask9 wojtask9 mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andresgalante andresgalante Awaiting requested review from andresgalante

1 more reviewer

@vsn4ik vsn4ik vsn4ik left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

css v4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wojtask9 @NielsHolt @mdo @vsn4ik @Johann-S