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

Disable use of ResizeObserver for menu auto-updating #5381

Merged
merged 2 commits into from Oct 16, 2022

Conversation

Methuselah96
Copy link
Collaborator

@Methuselah96 Methuselah96 commented Oct 14, 2022

Resolves #5256 (comment).

We do not have an official browser support policy, but it would be nice to delay relying on ResizeObserver for a major bump and make our official browser support policy clear in the next major.

I do not believe we anticipate the ResizeObserver getting triggered in many situations anyway since the menu itself shouldn't be resizing. The scroll events are much more critical to auto-updating working properly.

Resolves #5256 (comment).

We do not have an official browser support policy, but it would be nice to delay relying on `ResizeObserver` for a major bump and make our official browser support policy clear.

I do not believe we anticipate the `ResizeObserver` getting triggered in many situations anyway since the menu itself shouldn't be resizing. The scroll events are much more critical to auto-updating working properly.
@changeset-bot
Copy link

changeset-bot bot commented Oct 14, 2022

🦋 Changeset detected

Latest commit: 86bf2bf

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 Oct 14, 2022

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 86bf2bf:

Sandbox Source
react-codesandboxer-example Configuration

@Methuselah96 Methuselah96 changed the title Disable use of ResizeObserver for menu auto-update Disable use of ResizeObserver for menu auto-updating Oct 14, 2022
@Methuselah96 Methuselah96 merged commit 0f6ef09 into master Oct 16, 2022
@Methuselah96 Methuselah96 deleted the disable-element-resize branch October 16, 2022 22:36
@github-actions github-actions bot mentioned this pull request Oct 16, 2022
@desbrowne
Copy link

Hi @Methuselah96

For the major bump release, I wonder if you could use the following so that older browsers could still be supported without requiring a polyfill?
{ elementResize: 'ResizeObserver' in window }

@Methuselah96
Copy link
Collaborator Author

Methuselah96 commented Oct 17, 2022

Thanks for the suggestion, I'm not even sure we have any reason to re-enable it at this point. I'm having trouble trying to think of a situation where the ResizeObserver would be triggered but the other event listeners (scroll and resize on parent) would not, so we might be able to just do without it, even in the next major.

@desbrowne
Copy link

Thanks. I'm not super familiar with MenuPlacer, but would async loading menu items or changing menu-item heights cause this?

@Methuselah96
Copy link
Collaborator Author

There are two parts of menu positioning:

  1. If menuPlacement="auto", determine whether it should go on the top or bottom. Also scroll the menu into view if possible.
  2. If using portalling (or fixed positioning), update the menu's absolute position based on the position of the control element.

This PR only addresses auto-updating the second part of menu positioning. I don't think we have determined yet whether we want to be auto-updating the first part.

The second part does not rely on the menu's height at the moment, so async loading menu items or changing menu-item heights would not affect the auto-updating of the menu position the way that it's currently implemented.

Rall3n pushed a commit to Rall3n/react-select that referenced this pull request Oct 19, 2022
* Disable use of ResizeObserver for menu auto-update

Resolves JedWatson#5256 (comment).

We do not have an official browser support policy, but it would be nice to delay relying on `ResizeObserver` for a major bump and make our official browser support policy clear.

I do not believe we anticipate the `ResizeObserver` getting triggered in many situations anyway since the menu itself shouldn't be resizing. The scroll events are much more critical to auto-updating working properly.

* Create new-sloths-wink.md
Methuselah96 added a commit that referenced this pull request Oct 20, 2022
Fixes #5403.

I mistakenly thought in #5381 that the resize event was placed on the control element, but's it really just on the scroll parents of the control element.

This PR enables `ResizeObserver` if it's available. The referenced issue demonstrates a good use-case for it.
Methuselah96 added a commit that referenced this pull request Oct 20, 2022
* Use ResizeObserver to update menu position if available

Fixes #5403.

I mistakenly thought in #5381 that the resize event was placed on the control element, but's it really just on the scroll parents of the control element.

This PR enables `ResizeObserver` if it's available. The referenced issue demonstrates a good use-case for it.

* Create tricky-books-design.md
Rall3n pushed a commit to Rall3n/react-select that referenced this pull request Oct 21, 2022
…on#5404)

* Use ResizeObserver to update menu position if available

Fixes JedWatson#5403.

I mistakenly thought in JedWatson#5381 that the resize event was placed on the control element, but's it really just on the scroll parents of the control element.

This PR enables `ResizeObserver` if it's available. The referenced issue demonstrates a good use-case for it.

* Create tricky-books-design.md
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.

None yet

3 participants