Escape on nested Overlays #1861
Conversation
|
size-limit report 📦
|
| for (let i = handlers.length - 1; i >= 0; --i) { | ||
| handlers[i](event) | ||
| if (!event.defaultPrevented) { | ||
| for (const handler of Object.values(registry).reverse()) { |
There was a problem hiding this comment.
Is the order of Object.values(registry) consistent/predictable?
There was a problem hiding this comment.
Would it make more sense to push and pop from a stack (array)?
There was a problem hiding this comment.
Yep! I found it more consistent than an array implementation I tried to build myself.
| setEditing(false) | ||
| setTitle(title) | ||
| event.preventDefault() // prevent Overlay from closing, this is what we recommend | ||
| // event.stopPropagation() // this also works and feels nicer to use |
There was a problem hiding this comment.
Maybe we should update this comment to explain that event.stopPropagation() doesn't work here yet
There was a problem hiding this comment.
In this case, it does work though because the order of handlers is predictable 😅
src/hooks/useOverlay.tsx
Outdated
| @@ -29,6 +29,12 @@ export const useOverlay = ({ | |||
| const overlayRef = useProvidedRefOrCreate<HTMLDivElement>(_overlayRef) | |||
| useOpenAndCloseFocus({containerRef: overlayRef, returnFocusRef, initialFocusRef, preventFocusOnOpen}) | |||
| useOnOutsideClick({containerRef: overlayRef, ignoreClickRefs, onClickOutside}) | |||
| useOnEscapePress(onEscape) | |||
|
|
|||
| /** We only want one overlay to close at a time */ | |||
There was a problem hiding this comment.
Nitpick: do we need a JSDoc-style comment here?
| /** We only want one overlay to close at a time */ | |
| // We only want one overlay to close at a time |
There was a problem hiding this comment.
yep, not sure what I had here. fixed now.
Forgot to include it in the PR
Fixes #1854
useOnEscapePressare triggering allescapeevents globally #1854)Does not fix in this PR, need to experiment more: