Close non-containing overlays when a new overlay is opened #1510
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| onClose: () => void | ||
| } | ||
|
|
||
| export const visibleOverlays: OpenOverlay[] = []; |
There was a problem hiding this comment.
We should make sure that this isn't exported from the package (e.g. index.ts).
| ) | ||
| ) | ||
| .add( | ||
| '2 popovers', |
There was a problem hiding this comment.
I am able to open both popovers at the same time. Is that intended?
There was a problem hiding this comment.
definitely not intentional and i had it fixed at one point
i'll check out what happened and try to get that to work correctly again
| <View width="500px"> | ||
| <Picker label="Test" autoFocus> | ||
| .add( | ||
| '2 pickers', |
There was a problem hiding this comment.
Was also able to open two pickers at the same time when clicking quickly...
| // Get props for the menu trigger and menu elements | ||
| let ref = React.useRef(null); | ||
|
|
||
| let shouldCloseOnInteractOutside = (element) => !ref?.current?.contains(element) ?? false; |
There was a problem hiding this comment.
Why is this prop needed? Would be nice to avoid making everyone writing an overlay pass it in...
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (isOpen === true && visibleOverlays.length > 1) { |
There was a problem hiding this comment.
@devongovett yep, I think I had it working for one, then changed for the other and forgot to retest. I can reproduce it and I can fix it for one or the other. I'm having trouble solving for both though. It's this line here
having it as 1 solves it for the hooks which have no delay
having it as 0 solves it for the components which have the OpenTransition with a delay of 0 (which gets queued)
without knowing the overlay this trigger is opening, it's hard to determine if it's in the visibleOverlays or not yet
Some thoughts on it so far:
I could pass in overlayRef, if it's undefined then i know we're dealing with the delayed case, but that changes the API unless I make it optional and then because it's checking undefined it's hard to know if it's omitted or just that it's not mounted yet.
We can't check the id of the last visible overlay, the id isn't on the ref, it's somewhere further down in the tree.
We can't pass the id into useOverlay for storage in the array of visibleOverlays because it's on the dialog or menu context, so we'd need to change an API to get it to the useOverlay hook.
I thought about waiting to remove previous overlays dependent on when the new overlay mounts, but if there's a delay bigger than 0 to show it, then that might get a little funny looking? I think it'd be messy to implement as it'd be a new piece of state i think.
|
replaced by new behaviors in #1970 |
3 participants
Closes #1155
Also makes use of
shouldCloseOnInteractOutsidewhich reduces the number of times onOpenChange gets called, see https://github.com/adobe/react-spectrum/pull/1510/files#diff-5861ee186d3230691b6ff7f9ee6864ee290647d89e983003e1edfff6fcd6aa5eL225✅ Pull Request Checklist:
📝 Test Instructions:
Added new stories in storybook with two menu/picker/etc in them, check those
🧢 Your Project: