Auto-update menu position when using menu portalling #5256
Conversation
🦋 Changeset detectedLatest commit: 2c89e0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
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 2c89e0e:
|
Methuselah96
changed the title
|
@ryan-castner Thanks for trying out the branch. I am not able to reproduce the issue you're experiencing. I've pushed a test of
You can try it out by checking out that branch, starting the docs with |
There was a problem hiding this comment.
This looks great @Methuselah96!
I couldn't replicate the issue that @ryan-castner mentioned.
I agree that a minor makes sense here 👍
|
This has been released in |
|
This is awesome, thank you so much for this change! I've been experiencing this issue in my wrapper |
|
Hi @Methuselah96, thank you for this change, but I can confirm it's a major release which introduces some breaking change since it relies on Thanks again for your work and I hope it helps. |
|
@alk-sdavid Which browsers are you trying to support? As far as I know we don't have any browser support policy at the moment besides not IE 11 (mentioned in the v5 upgrade guide). |
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.
|
Looks like Safari on iOS 12.5 and Android Browser 4.4.4 might be the two browsers that don't support it if we're aiming to support browsers with >.25% usage. I've created #5381 to disable the |
|
Hi, yes it was not IE11, I just tested react-select on browserstack with some browser which does not support ResizeObserver (https://caniuse.com/resizeobserver). I do not have any issue about it, but I was just thinking it would be better to make a major release and to make it clear in the changelog and the documentation. |
|
Hi @Methuselah96, I've just upgraded to the latest version Do you have any workaround on it? |
|
It should be resolved by #5381 which will hopefully be released in the next couple days. |
* Disable use of ResizeObserver for menu auto-update 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. * Create new-sloths-wink.md
|
@alk-sdavid @kimi-truong This should be resolved in |
|
@Methuselah96 thanks so much 🙏 , it works now. |
* yarn install * Add floating-ui * Make MenuPortal function component * Fix csstype resolution * Update * Remove unnecessary export * Avoid ResizeObserver * Create soft-bags-shave.md * Include fixed * Update * Update * Fix * Bump @floating-ui/dom * Format
* 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
Fixes #3533.
Fixes #4088.
Fixes #4159.
Fixes #4336.
This PR makes it so that when using menu portalling, event listeners are attached to the scrolling parents of the Select component in order to update the position after scroll or resize of one of its parents. This is accomplished using Floating UI's
autoUpdatefunction and is heavily inspired by theiruseFloatinghook (but doesn't directly use it in order to make this more minor of a change).Previously I was trying to rewrite all of menu positioning, but I decided to go with this more modest approach to get this resolved more quickly and make it less of a breaking change. However a complete rewrite of menu positioning might still happen in the future if proven necessary.
I attempted to make this change as backwards compatible as possible. The only change should be that there are event listeners attached to the Select component's scroll parents if using menu portalling. It could be argued that this is either a bug fix, a new feature, or a breaking change. My leaning would be to release this in a minor version since I can't think of a use case where one would not want the menu position to update when the Control element moves.