🦋 Changeset detected

Latest commit: 8199f19

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

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

✅ Deploy Preview for vibrant-gates-22c214 ready!

Name	Link
🔨 Latest commit	8199f19
🔍 Latest deploy log	https://app.netlify.com/sites/vibrant-gates-22c214/deploys/66dfb5b7f262ad0008115e0b
😎 Deploy Preview	https://deploy-preview-3042--vibrant-gates-22c214.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Are you sure that this did not break what is documented here?

// Once `isOpen` flips to `true`, `isPositioned` will switch to `true`
// asynchronously. We can use an Effect to determine when it has
// been positioned.
useEffect(() => {
  if (isPositioned) {
    // ...
  }
}, [isPositioned]);

isPositioned is no longer changing

isPositioned is no longer changing

It works locally just fine, which means you might be experiencing an edge case where you were relying on the old behavior in some scenario. The only thing I can think of is update() for positioning is being called before your open state changes, which sets a ref inside the layout effect, but I'm not sure when/how this can happen given update() is always called in a later effect.

Can you provide a reproduction where it fails?

Okay, I know where it can fail, but it seems to highlight improper usage.

It can happen when combining these usage scenarios:

1. Not using conditional rendering, so the floating element is always present when the component is mounted.
2. Using whileElementsMounted: autoUpdate (which isn't supported when using case 1 above), or not specifying autoUpdate at all.
3. If not using autoUpdate at all, also not calling update() when open changes to true. This is the minimum requirement for this new logic to work (and also for the positioning to work reliably in general).

I would say this isn’t technically a regression but rather a bug fix that caused code relying on the old behavior to unfortunately start breaking unexpectedly (something semver isn't capable of handling). However it actually highlights an issue in the consumer code because the positioning wasn’t implemented correctly before.

When not using conditional rendering, this is the correct way to use autoUpdate:

useLayoutEffect(() => {
  if (open && elements.reference && elements.floating) {
    return autoUpdate(elements.reference, elements.floating, update);
  }
}, [elements.reference, elements.floating, open, update]);

Or at the very least, when not using autoUpdate:

useLayoutEffect(() => {
  if (open) {
    update();
  }
}, [open, update]);

This not only makes isPositioned work as expected, but also fixes a bug with the positioning implementation as well.

@atomiks Thanks for clarifying.

When not using conditional rendering tho if open is set to true and then false, would you expect to have isPositioned as false?

Yes because even though it was already positioned and remains mounted, the position can become "stale" while it's closed. It's possible that the next open has a drastically different position where you'll need to wait for isPositioned once again to do focus or scrolling since the old position is now out of view. e.g. reference changed its location on the screen due to scrolling containers or something else.

ok this is weird to me and it was not like this before (if it was a bug or not whatever)

To me it should return isPositioned: true even if the pos did not change from previous