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

[Popper] Leverage ResizeObserver #22303

Open
ohlr opened this issue Aug 21, 2020 · 8 comments
Open

[Popper] Leverage ResizeObserver #22303

ohlr opened this issue Aug 21, 2020 · 8 comments
Labels
component: Popper The React component. See <Popup> for the latest version. new feature New feature or request

Comments

@ohlr
Copy link
Contributor

ohlr commented Aug 21, 2020

https://codesandbox.io/s/practical-kalam-jz2ew?file=/src/Demo.js

open the popper by clicking on the button

  1. When scrolling (fast) it does not remain in position but jiggles around
  2. When using "keepMounted" open the popper, close it -> additional white space at bottom of page
  3. When the height of the content inside the popper increases (i.e. because of a form interaction) the position of the popper is not updated until scrolling. After scrolling it jumps up to accommodate for the new height.
@ohlr ohlr added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 21, 2020
@oliviertassinari oliviertassinari added the component: Popper The React component. See <Popup> for the latest version. label Aug 21, 2020
@oliviertassinari
Copy link
Member

  1. An issue with https://github.com/popperjs/popper-core (but I can't reproduce)
  2. By definition
  3. Interesting. It sounds like we should document how you can use the popperRef to update the position of the positioning or start using the resize observer [Tabs] Update position when layout change (ResizeObserver) #9337 (comment)

@oliviertassinari oliviertassinari added new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 21, 2020
@oliviertassinari oliviertassinari changed the title Popper rendering issues [Popper] Improve position update Aug 21, 2020
@mlizchap
Copy link
Contributor

mlizchap commented Aug 24, 2020

For #1 you can add the following popperOptions to the Popper component:

popperOptions={{
    positionFixed: true,
}}

and this should fix this problem (https://codesandbox.io/s/practical-kalam-jz2ew?file=/src/Demo.js:2167-2232)

with Material-UI v5 (Popper v2), the option is strategy: 'fixed' https://popper.js.org/docs/v2/constructors/#strategy.

@mlizchap
Copy link
Contributor

mlizchap commented Aug 24, 2020

For 3. you could also just let the parent Popper component know when it's Child component updates using a callback function - https://codesandbox.io/s/loving-water-b9950?file=/src/Demo.js. You could also just nest the Child component in the Popper component too. Not sure if that would be more efficient than using a Ref but it might be easier.

@ohlr
Copy link
Contributor Author

ohlr commented Aug 24, 2020

@mlizchap Thank you for the suggestions. While the first one is really helpful (and should probably be included in the docs) using the second one feels a bit weird - For example I would not expect that I have to pass an update to a Card when it's child is changing. Also in my case it's a bit harder as I deal with a form which updates in multiple occasions not just on a button click.

@oliviertassinari regarding 2. what do you mean by definition? inside the docs it says: "Always keep the children in the DOM." - I would expect that the popper gets hidden simply but not that it adds whitespace at the bottom of the page.. Is there any way to prevent it?

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 25, 2020

  1. This is a concern for popper, not us, we already mention them in the documentation, it's up to developers to dive deeper.
  2. It's up to developers to hide the popper if it's the behavior they desire.
  3. I agree that asking to rerender is a workaround, not a proper solution.

To clarify the "improvement" I have in mind behind the label: look into using ResizeObserver to avoid having to manually get a ref on popper and update the position as we used to do in the Slider demo or Rating demo.

@oliviertassinari oliviertassinari changed the title [Popper] Improve position update [Popper] Leverage ResizeObserver Aug 25, 2020
@ohlr
Copy link
Contributor Author

ohlr commented Aug 25, 2020

Thank you for clarification. Wrapping the inner of the popper with
<Box display={open ? "block" : "none"}></Box>
as demonstrated here: https://codesandbox.io/s/nervous-cloud-s2x4p?file=/src/Demo.js:2264-2316 does the job

@hbjORbj
Copy link
Member

hbjORbj commented Aug 16, 2021

As far as I understand from the previous conversation on this issue, 1 and 2 have already been resolved, and 3 is to be fixed. Am I correct?

I cannot reproduce 3 as shown in the following recording.

Screen.Recording.2021-08-16.at.09.41.00.mov

https://codesandbox.io/s/empty-sun-8js8u?file=/src/Demo.js

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 16, 2021

@hbjORbj I have added a link to reproduction from your video. popper.js makes it work in v2 by using inset.

We can still reproduce when the content overflows: https://codesandbox.io/s/spring-tdd-2zxjw?file=/src/Demo.js

overflow.mp4

It's not really important to fix, but if the fix is low overhead, then why not :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popper The React component. See <Popup> for the latest version. new feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants