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(menu): Fixed maxHeight calculation for menu portal. #5716

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elliotcourant
Copy link

If the viewSpaceBelow is less than 300px (default max height):

The first time the menu renders, the viewSpaceBelow will probably not be greater than the menuHeight. So it will take path 2. or 3. for bottom/auto. If the viewable area is greater than the minimum height then it will take path 3.
On the next render, the menuHeight is now less than viewSpaceBelow because it was constrained by the third path. So the first path is taken, but preferredMaxHeight will be greater than viewSpaceBelow because path 3 was taken on the previous render. The menu renders but overflows outside the view.
On the following render, path 1 is not taken because the height is greater than the viewSpaceBelow again (because its overflowing). So path 3 is taken again.

This resolves the potential for flickering by making sure that the size that would be rendered will actually fit. But comparing the viewSpaceBelow and scrollSpaceBelow to the maxHeight that would actually be returned, not to the height of the menu currently.


TLDR; When the space the menu would be rendered in is less than the maxHeight, but greater than the minHeight; the menu will flicker because it alternates the maxHeight value because of how it checks space available. This changes it so that the check is against what the new height would be, instead of against what it is now.


If needed I can provide a video of the behavior; its absolutely possible I'm doing something wrong here. But the fact that menuHeight could be less than preferredMaxHeight seems like this is still a bug.

If the viewSpaceBelow is less than 300px (default max height):

The first time the menu renders, the viewSpaceBelow will probably not be
greater than the menuHeight. So it will take path 2. or 3. for
bottom/auto. If the viewable area is greater than the minimum height
then it will take path 3.
On the next render, the menuHeight is now less than viewSpaceBelow
because it was constrained by the third path. So the first path is
taken, but preferredMaxHeight will be greater than viewSpaceBelow
because path 3 was taken on the previous render. The menu renders but
overflows outside the view.
On the following render, path 1 is not taken because the height is
greater than the viewSpaceBelow again (because its overflowing). So path
3 is taken again.

This resolves the potential for flickering by making sure that the size
that _would_ be rendered will actually fit. But comparing the
`viewSpaceBelow` and `scrollSpaceBelow` to the `maxHeight` that would
actually be returned, not to the height of the menu currently.
@changeset-bot
Copy link

changeset-bot bot commented Jul 30, 2023

🦋 Changeset detected

Latest commit: 1b3571b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-select Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 30, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1b3571b:

Sandbox Source
react-codesandboxer-example Configuration

@elliotcourant
Copy link
Author

Bug

Each hover causes a re-render, causing the height to be recalculated.

react-select-bug-2023-07-30_17.22.25.mp4

Patched

The menu will not be larger than the space available.

react-select-fixed-2023-07-30_17.23.48.mp4

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

Successfully merging this pull request may close these issues.

2 participants