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

Fix Popper preventOverflow boundary config #32845

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

kyletsang
Copy link
Contributor

Currently, the boundary config is being assigned to the wrong var (rootBoundary) in the popper config. It should be assigned to the boundary var in popper's config.

Ref: https://popper.js.org/docs/v2/utils/detect-overflow/#boundary

@kyletsang kyletsang requested a review from a team as a code owner January 19, 2021 20:53
@XhmikosR
Copy link
Member

@kyletsang do we have any issue about this? I don't suppose this issue could be related to any of the other Popper issues we have in v5?

@kyletsang
Copy link
Contributor Author

@XhmikosR, currently no issue reported. I was reviewing the code to see if there's any new modifiers I needed to port over to react-bootstrap when I stumbled on it.

This could potentially cause issues due to incorrect usage of rootBoundary. Unfortunately, I haven't tested in-depth though.

@XhmikosR
Copy link
Member

@kyletsang do you think you could add some tests about this?

@kyletsang
Copy link
Contributor Author

Ugh, I think I'm going to need some assistance on this. I can't seem to come up with a demo that shows any difference when setting an incorrect rootBoundary vs a correct one.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 1, 2021

Unfortunately, I have no idea how to test for this. We should have a test to make sure we don't hit this again, but if we can't have a test, I guess we could make an exception here.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 3, 2021

@rohit2sharma95 LGTY?

@XhmikosR XhmikosR merged commit d63a922 into twbs:main Feb 4, 2021
@kyletsang kyletsang deleted the fix/popper-preventoverflow branch February 5, 2021 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants