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

Popover does not correctly update position on content resize #1737

Closed
kevinstadler opened this issue Jan 5, 2020 · 7 comments
Closed

Popover does not correctly update position on content resize #1737

kevinstadler opened this issue Jan 5, 2020 · 7 comments

Comments

@kevinstadler
Copy link

  • components: Popover, UncontrolledPopover
  • reactstrap version 8.2.0
  • react version 16.12.0
  • (popper.js version 1.16.0)

What is happening?

Popover doesn't always update its position when dynamic changes in the popover content change the size of the popover. In particular when placement="top" the popover stays in the same position relative to the target prop as when it was originally opened, which makes it end up either covering the target prop, or being too far away from it. (This issue is related to but not the same as #1404, and also not the same as #563, which is about window resizing.)

What should be happening?

Whenever there are dynamic changes to the content of a Popover or UncontrolledPopover, reactstrap should signal this on to the popper.js component so that it can update/reposition accordingly (see floating-ui/react-popper#60 )

Steps to reproduce issue / Code

Code example using a Collapse component inside the Popover: https://codesandbox.io/s/relaxed-architecture-bwtq1

Live example: https://bwtq1.csb.app

Suggested fix

As documented in floating-ui/react-popper#60 the Popper component provides access to a scheduleUpdate method that can be called when the component should reposition. This method appears to be called correctly for some of the other placement options, but not 'top', so I wonder if it just an issue of timing when it is called, i.e. if the animation of the Collapse component has something to do with it?

@kyletsang
Copy link
Member

I think the best option would be to expose scheduleUpdate as a render prop for anything that inplements popper.

Thoughts @TheSharpieOne?

@TheSharpieOne
Copy link
Member

Sounds good to me.
Also, there is a new version of popper.js, so we might want to upgrade.

@jrozbicki
Copy link

@TheSharpieOne will this be looked at? Looking forward for this change

@barbalex
Copy link

barbalex commented Apr 6, 2020

Please remember that users will often install react-popper and thus popper.js or @popperjs/core (https://github.com/popperjs/react-popper#install) separately from reactstrap (https://github.com/reactstrap/reactstrap#optional-dependencies).

So I assume updating popper.js to @popperjs/core would be a breaking change for them.

@jrozbicki
Copy link

@TheSharpieOne can this be resolved without updating dependecies?

@kyletsang
Copy link
Member

The linked PR does not update any dependencies.

@kyletsang
Copy link
Member

Availabie in 8.5.0

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

5 participants