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

Incorrect positions with placement auto #375

Open
jquense opened this issue Jul 10, 2020 · 5 comments
Open

Incorrect positions with placement auto #375

jquense opened this issue Jul 10, 2020 · 5 comments

Comments

@jquense
Copy link
Contributor

jquense commented Jul 10, 2020

Reproduction demo

https://codesandbox.io/s/react-popper-v2x-issue-template-vb3kt?file=/src/index.js

Steps to reproduce the problem

  1. ensure the codesandbox has enough room along both axis' so that auto move it to the side as well as bottom/top
  2. notice an axis while scrolling change calculates the wrong offsets (further scrolling fixes it)

The issue is that the new arrow position makes the popper longer/shorter/wider/etc

What went wrong?

This behavior makes sense to me abstractly, position changes require the popper to render with the new position in order to be the correct size. What i don't understand is why i don't usually see this with the non react version, and is there a general why to handle this? In React-overlays we have an effect that checks if the placement changed in state, and triggers a forceUpdate but this leads to situations where it loops indefinitely in narrow cases where the new placement also causes a placement change.

Any other comments?

It seems like modifiers do something to avoid these loops, but i'm not sure if the same thing is possible to do outside of them in react-land. Also how Tippy seem to avoid this?

@jquense
Copy link
Contributor Author

jquense commented Jul 10, 2020

Realizing the reason this works, is that most examples use offsets instead of padding to make room for the arrow yeah?

@atomiks
Copy link
Collaborator

atomiks commented Jul 11, 2020

The issue is that the new arrow position makes the popper longer/shorter/wider/etc

You can't use margin to offset the popper because it changes its size which causes problems like the flip loop. You can only use the offset modifier.

It also happens in the core version, see https://popper.js.org/docs/v2/migration-guide/#5-ensure-your-popper-and-arrow-box-size-is-constant-for-all-placements

@jquense
Copy link
Contributor Author

jquense commented Jul 12, 2020

I know that margin doesn't work, the example here doesn't use margin. The unclear part here is you also can't use padding for accomplish this, which isn't really covered anywhere (it'd be hard to warn about for obvious reasons i know).

Doing markup like the code sandbox is very common, and not obviously an example of using margin to offset the popper, tho that is what is happening in practice. Overall it seems like the real restriction is that the popper element should not change dimensions because of a placement?

@atomiks
Copy link
Collaborator

atomiks commented Jul 13, 2020

the example here doesn't use margin

It does? Not on the popper itself but an inner element that mimics padding. That's the same thing as far as Popper is concerned with this issue (changing its dimensions based on the placement).

Screen Shot 2020-07-13 at 4 38 36 pm

Overall it seems like the real restriction is that the popper element should not change dimensions because of a placement?

Correct, you can't conditionally change the popper's size based on the placement, it's a limitation

@jquense
Copy link
Contributor Author

jquense commented Jul 15, 2020

It does? Not on the popper itself but an inner element that mimics padding.

Right, that was my (pedantic) point the popper element does not use margin, I see now that it's effectively the same thing using padding on the inside. I'm not trying to be difficult, i'm sharing how my brain thought about this. reading https://popper.js.org/docs/v2/migration-guide/#4-remove-all-css-margins and seeing the console warnings and it wasn't obvious to me that "padding on an inner element" was actually the same thing until I wrote out the issue and something clicked in my head. In retrospective duh.

So I guess i'm just suggesting that the docs/warning aren't as being as clear as they could be about what the happy path is for styling.

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

No branches or pull requests

2 participants