Use Popper for position calculations #181

taion · 2017-06-10T20:23:07Z

There's a functional change here in same-axis positioning when the positioned element is about to escape. Otherwise I think everything mostly works the same. There's a debounce on the initial update, but that's not really noticeable.

I changed the API a bit to make it maybe cleaner. Plus now I explicitly hide the positioned element when we don't have a position for it.

We're not really exposing all that Popper is offering, but I'm okay with that. This closes a lot of the positioning issues on our end.

Closes #52
Closes #135
Closes #175
Closes #176
Closes #178

taion · 2017-06-10T20:27:09Z

test/PositionSpec.js

+      });
+    }
+
+    [


these are a bit different now in the "offsetBefore" and "offsetAfter" cases

taion · 2017-06-10T20:27:19Z

test/PositionSpec.js

+          arrowPosition: { top: 100 },
+        },
+        offsetBefore: {
+          position: { left: 50, top: 0 },


taion · 2017-06-10T20:28:09Z

test/PositionSpec.js

-      {
-        placement: 'left',
-        noOffset: [50, 200, undefined, '50%'],
-        offsetBefore: [-200, 50, undefined, '0%'],


corresponding old...

though note that the target position is a bit different now

the old impl would only apply the padding to cross-axis positioning. this on the other hand applies the padding to same-axis, mostly

taion · 2017-06-10T21:21:40Z

Hmm, this makes the R-B docs page look weird even with fixes. Meh.

jquense · 2017-06-10T22:03:14Z

Browser support? Also what is the motivation for this? I'm a big fan tho I thought we were holding off b/c of various IEs

taion · 2017-06-10T22:34:16Z

Looks like IE10+: https://saucelabs.com/beta/builds/07fe610fcdca470f8ca45384a15409ff

No particular motivation... just saw we had a lot of position issues/PRs open, and wondered if we could scrap all of our own code. It was harder than expected and I got pretty carried away, but that happens.

Unfortunately this might not be viable – Popper takes into consideration e.g. margins on the popped elements, while TWBS3 popovers (and maybe tooltips as well?) are explicitly built with the assumption that such margins are ignored.

Masalovych · 2017-06-14T05:28:34Z

Hi,
When we can expect this pull-request will be merged?

jquense · 2017-08-11T14:00:26Z

@taion is this designed to account for TWB v3 reqs? I'd say we add this but ignore the v3 stuff and keep both the new and old Position around for v3 legacy but retire it here. v4 bootstrap needs the popper version anyway

taion · 2017-08-11T16:34:18Z

This won't work with TWBSv3 without horrible hacks because of the way TWBSv3 handles margins and such.

You proposed moving the TWBSv3-compatible version of this into React-Bootstrap, which sounded like a good idea at the time.

jquense · 2017-08-11T16:38:55Z

I'm hitting a point just in our own code where I jsut want to use popper and not the default bootstrap stuff, so i'm a bit anxious to move on that.

I think we just freeze the position component where it is and either move it to RB or keep it here as PositionLegacy, and move forward with the unhacky popper implementation. But importantly we don't try and make popper work with bootstrap v3

taion · 2017-08-11T16:40:19Z

I'd move it to R-B. It's really very TWBSv3-specific, and I don't think it works too well for other cases. Or anyway the Popper-based version is more generic.

taion · 2017-08-11T16:40:55Z

It's worth considering whether we want to use react-popper, though: https://github.com/souporserious/react-popper

The API is pretty different from what's here, but... well, it's there.

jquense · 2017-08-11T16:41:50Z

That works for me, I'll move it over. Does this need any unhacking, or you think its good to go as a general implementation over Popper?

taion · 2017-08-11T16:44:03Z

Config-wise, I tried to get something that pragmatically matched how TWBSv3 worked, with things like: https://github.com/react-bootstrap/react-overlays/pull/181/files#diff-b7dd97a1efefd56a539d6f75fcd04a7bR209

If we're not worrying about TWBSv3-compat any more, then I'd switch to either using better defaults, or to exposing those config flags.

bcullman · 2017-08-14T22:05:05Z

Any chance we would be getting edge detection for free by using popper/react-popper here?

taion · 2017-08-14T23:37:54Z

Yes.

jeffredodd · 2017-08-31T16:41:35Z

Any progress on this? I'd love to help out but i'm not quite sure where this PR stands?

taion · 2017-09-01T15:08:44Z

Next step is to move the legacy implementation into React-Bootstrap, because this doesn't work well with TWBSv3.

devongovett · 2017-09-18T20:38:57Z

IMO it would be nice not to have to bring in another large dependency just for positioning. Is it really needed? What do we get that we wouldn't get from e.g. #175?

taion · 2017-09-18T20:45:37Z

Well, it's 6 kB minified + gzipped, so not that big? Also just more powerful/flexible.

devongovett · 2017-09-18T21:09:39Z

6K is pretty big for what it is doing I think. What actual features does it provide that you're exposing in react-overlays other than fixing cross-axis positioning?

jquense · 2017-09-19T01:11:13Z

we are thinking about exposing all the features, thats sort of the point and benefit. :) In any case this is likely to happen for bootstrap v4 because upstream bootstrap uses it

devongovett · 2017-09-19T18:41:11Z

@jquense is the plan to merge this soon then? I need some sort of cross-axis positioning, whether that be #175 or this.

jquense · 2017-09-19T19:27:29Z

I'd love to merge it asap, but I don't have time at the moment to adjust the PR here per the above convo and I'm guessing @taion may be in a similar boat

devongovett · 2017-09-19T19:46:44Z

Also is this backwards compatible with the existing Position component? If not, what is the migration path? Major version bump? We could merge #175 in the mean time which is backwards compatible...

jquense · 2017-09-19T19:58:55Z

It doesn't need to be backwards compatible here. But we need to move the existing Position component to react-bootstrap per the convo above

devongovett · 2017-09-19T20:04:52Z

How will that work since Overlay here uses Position here as well? We'd need to copy Overlay as well then, no? Or maybe make a prop to pass in the component to use... that could be useful for other custom positioning cases anyway I suppose.

adilapapaya · 2017-11-16T22:54:07Z

@taion and @jquense would it be possible to merge #175 for the time being and save the popper integration for a larger version bump? I'm in the same boat as @devongovett – need some cross-axis positioning functionality.

p.s. Thanks for the awesome library + work so far!!

jquense · 2018-01-10T15:08:46Z

@taion yo can we just merge this?

taion · 2018-01-10T16:37:07Z

Well, no, because package-lock.json has a merge conflict :p

taion · 2018-01-10T16:37:21Z

Oops finger slipped. I can look for some time to work on this though.

kumarharsh · 2018-01-11T06:13:33Z

Oops finger slipped. I can look for some time to work on this though.

Thank god for the finger slip! I thought for a moment this was dead 😛
Would love it if react-overlays does support Popper - right now I'm using react-overlays as well as popper, both of them doing similar work, just that popper provides better positioning, and react-overlays provides all those other things (RootCloseWrapper, Portal, etc).

jquense · 2018-02-28T13:02:46Z

@taion imma merge this later today

taion · 2018-02-28T16:09:15Z

Sounds good, do you need anything from me?

jquense · 2018-02-28T16:19:00Z

It'd be excellent if you had a moment to see if this properly sets us up for the v4 dropdowns.

taion commented Jun 10, 2017

View reviewed changes

taion closed this Jan 10, 2018

taion reopened this Jan 10, 2018

taion added 2 commits March 5, 2018 13:49

Use Popper for position calculations

1e2b00f

Forward props to child

c2684fe

jquense force-pushed the popper branch from e43af84 to c2684fe Compare March 5, 2018 18:51

jquense changed the base branch from master to v1-dev March 5, 2018 18:53

jquense merged commit 3565373 into v1-dev Mar 5, 2018

jquense deleted the popper branch March 5, 2018 18:55

bluetidepro mentioned this pull request Apr 26, 2018

Auto change placement of Position when overlapped by the browser window #178

Closed

+                    });
+                  }
+                  [

Use Popper for position calculations #181

Use Popper for position calculations #181

Conversation

taion commented Jun 10, 2017 • edited Loading

taion Jun 10, 2017

Choose a reason for hiding this comment

taion Jun 10, 2017

Choose a reason for hiding this comment

taion Jun 10, 2017

Choose a reason for hiding this comment

taion commented Jun 10, 2017

jquense commented Jun 10, 2017

taion commented Jun 10, 2017

Masalovych commented Jun 14, 2017

jquense commented Aug 11, 2017

taion commented Aug 11, 2017

jquense commented Aug 11, 2017 • edited Loading

taion commented Aug 11, 2017

taion commented Aug 11, 2017

jquense commented Aug 11, 2017

taion commented Aug 11, 2017

bcullman commented Aug 14, 2017

taion commented Aug 14, 2017

jeffredodd commented Aug 31, 2017

taion commented Sep 1, 2017

devongovett commented Sep 18, 2017

taion commented Sep 18, 2017

devongovett commented Sep 18, 2017

jquense commented Sep 19, 2017

devongovett commented Sep 19, 2017

jquense commented Sep 19, 2017

devongovett commented Sep 19, 2017

jquense commented Sep 19, 2017

devongovett commented Sep 19, 2017

adilapapaya commented Nov 16, 2017

jquense commented Jan 10, 2018

taion commented Jan 10, 2018

taion commented Jan 10, 2018

kumarharsh commented Jan 11, 2018 • edited Loading

jquense commented Feb 28, 2018

taion commented Feb 28, 2018

jquense commented Feb 28, 2018

taion commented Jun 10, 2017 •

edited

Loading

jquense commented Aug 11, 2017 •

edited

Loading

kumarharsh commented Jan 11, 2018 •

edited

Loading