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

Update Popper to v2.x #31178

Merged
merged 10 commits into from
Dec 6, 2020
Merged

Update Popper to v2.x #31178

merged 10 commits into from
Dec 6, 2020

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Jun 25, 2020

  • Docs
  • Changes in Popover css to add a bit of space between the element and the popover
  • Update to the latest popper.js version
  • Conflict between Popper.js and our css, which block us to align right dropdown
    • Check responsive behaviour
    • Check behaviour in navbar
  • Document the fallbackPlacement -> fallbackPlacements breaking change?
  • Tighten bundlewatch limits before the final merge
  • Update Popper docs links to v2 (search for https://popper.js.org/docs/ but don't blindly replace them!)
  • Add more JS tests to cover the changes in this branch

Fixes #29842

Preview: https://deploy-preview-31178--twbs-bootstrap.netlify.app/

nuget/bootstrap.nuspec Outdated Show resolved Hide resolved
js/src/tooltip.js Outdated Show resolved Hide resolved
@Johann-S
Copy link
Member Author

Hi our @twbs/css-review team ❤️

Would you help me here ? I have some css issues on this migration

@MartijnCuppens
Copy link
Member

Changes in Popover css to add a bit of space between the element and the popover

Is this just a matter of setting the offset in popper?

Conflict between Popper.js and our css, which block us to align right popover

What is this exactly? is it the alignment of the arrow?

@Johann-S
Copy link
Member Author

Is this just a matter of setting the offset in popper?

I tried to do that, but it does nothing 😟 so I think with a bit of CSS we can change that easily to avoid that:

image

What is this exactly? is it the alignment of the arrow?

Sorry I meant on our dropdown. See:

image

@jquense
Copy link
Contributor

jquense commented Jul 10, 2020

Hey folks, we tried to preemptively upgrade to popper v2 in react-bootstrap for bootstrap 4 and thought it might be helpful top share what we learned:

tl;dr; Proper popper v2 support requires bigger CSS changes to the tooltip, popover, and dropdown menu

The slightly longer version is that popper no longer considers margins when calculating offsets, which ius why the Popover and dropdown are positioned incorrectly. To fix, all distance should be set via the popper offset modifier.

Another related consequence to this is that arrow positioning that depends on creating space via a padding (the tooltip) also doesn't work well anymore (maybe never). The issue there is that when using auto positions or flip the dimensions of the popper element will change if the tooltip is moved to a different axis (b/c x and y padding is changed). Because the offsets were calculated uses the old box, the newly flipped tooltip will be incorrectly positioned until popper runs again.

Popper docs and Tippy both avoid this problem by only using the offset modifier and offsetting the arrow relative to the container (e.g left ; -4px)

None of this is particularly hard to fix BUT it has the consequence of needing config for arrow size available in JS, or doing some getComputedStyle shenanigans in a offset modifier (which does accept a function but doesn't pass in the actual popper element).

@jquense
Copy link
Contributor

jquense commented Aug 8, 2020

I'd also add that I'd be happy to help out getting this in. Popper v2 is awesome and tiny

@mdo
Copy link
Member

mdo commented Sep 17, 2020

@jquense We'll take any help you can give! Would love to get this included in Alpha 3!

@XhmikosR XhmikosR changed the title move to popperjs v2 Update to popperjs v2.x Oct 1, 2020
@XhmikosR XhmikosR changed the title Update to popperjs v2.x Update to popper.js v2.x Oct 1, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Oct 1, 2020

Rebased it, hopefully nothing broke :)

@XhmikosR XhmikosR force-pushed the feat/popper-v2 branch 3 times, most recently from 1aef9d4 to fb16dcc Compare October 1, 2020 06:29
@jquense

This comment has been minimized.

@XhmikosR XhmikosR force-pushed the feat/popper-v2 branch 3 times, most recently from c3b0d48 to 047fc69 Compare December 4, 2020 08:27
js/src/dropdown.js Outdated Show resolved Hide resolved

// Reset positioning when positioned with Popper
&[style] {
right: auto; // TODO RTL?
Copy link
Member

Choose a reason for hiding this comment

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

This needs a check @ffoodd for RTL (and the whole PR)

Copy link
Member

@ffoodd ffoodd Dec 4, 2020

Choose a reason for hiding this comment

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

On the RTL side, this will be translated to left: auto.
If this part is meant to reset initial positionning, maybe we should reset both sides? If so, it'll need #{"/* rtl:ignore */"} as seen a few lines below.

Edit: in fact it seems to behaves as expected. Only the arrows aren't flipped.

@XhmikosR XhmikosR force-pushed the feat/popper-v2 branch 2 times, most recently from e0b7537 to de8cd00 Compare December 4, 2020 14:27
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

On the RTL part, arrows aren't flipped in RTL so there's something to tackle there.

After trying things out, I think .popover-arrow should be processed by RTLCSS (meaning drop #{"/* rtl:ignore */"} in every .bs-popover-* > .popover-arrow { … }).


// Reset positioning when positioned with Popper
&[style] {
right: auto; // TODO RTL?
Copy link
Member

@ffoodd ffoodd Dec 4, 2020

Choose a reason for hiding this comment

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

On the RTL side, this will be translated to left: auto.
If this part is meant to reset initial positionning, maybe we should reset both sides? If so, it'll need #{"/* rtl:ignore */"} as seen a few lines below.

Edit: in fact it seems to behaves as expected. Only the arrows aren't flipped.

@XhmikosR XhmikosR merged commit 9312442 into main Dec 6, 2020
@XhmikosR XhmikosR deleted the feat/popper-v2 branch December 6, 2020 16:42
@XhmikosR XhmikosR mentioned this pull request Dec 6, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Popper.js to v2.x
8 participants