adobe / adobe/react-spectrum

useOverlayPosition: strange behaviour of shouldUpdatePosition

Open
#4,477 8 comments 0 reactions 0 assignees View on GitHub
Overlay Positioning Overlays
Dominant language
TypeScript
Stars
15.9k
Forks
1.6k
Avg merge
3d 9m
Merged PRs (30d)
59

Description

Hey!

I think that the naming of `shouldUpdatePosition` is a little bit confusing. As I imagine, it should calculate position once and then ignore all resizes, scrolls, etc on subsequent updates

But according to line:
https://github.com/adobe/react-spectrum/blob/273e828cd17c3d229e8ab10ce62bd0a6f0f43963/packages/%40react-aria/overlays/src/useOverlayPosition.ts#L128

If shouldUpdatePosition is false, it will never calculate position even for initial render. I believe it should work like `eventListeners` modifier of Popper.js (https://popper.js.org/docs/v2/modifiers/event-listeners/)

This behaviour leads bug/behaviour like this:
https://codesandbox.io/s/great-mendel-l0c8ye?file=/src/App.js
wrong initial positioning with shouldUpdatePosition=false, since useOverlayPosition does nothing useful in this case

If you're agree with it and it won't break anything, let me submit a PR where positioning is calculated on first render anyway and event listeners are added only when shouldUpdatePosition=true :)

Contributor guide

Open the contributing guide

Research direction

Start in packages/@react-aria/overlays/src/useOverlayPosition.ts at the referenced line 128, then reproduce the initial-positioning problem with the linked CodeSandbox. Verify the intended behavior for shouldUpdatePosition=false: initial positioning should still occur, while later resize and scroll updates should be controlled by that option.

Written by the indexing model from the issue text.

Assessment

Tech stack
react, typescript
Domain
frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.