Fix Menu's Appearing Behind Home Content#394
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Portal primitive to lift canvas elements above tldraw's stacking context, refactors the Menu component, and updates the useFocusTrap and useKeydown hooks to improve keyboard navigation. Key feedback includes: avoiding a global capture-phase listener in useKeydown to prevent breaking event propagation, optimizing the Portal component to prevent DOM recreation and state loss on className changes, retaining the focus trap in the Menu component for accessibility, and refining the tabbable elements selector in useFocusTrap to support all non-negative tab indices.
nick-nlb
left a comment
nick-nlb
left a comment
There was a problem hiding this comment.
Thank you Paulo - a couple of comments before merging, but otherwise looks great.
|
|
||
| useEffect(() => { | ||
| const element = document.createElement('div'); | ||
| if (className) element.className = className; |
There was a problem hiding this comment.
This effect really does two things. Assuming that the className might change independently of the initial mounting, this triggers the mounting (ie.. setMountedElement) when the class changes. We can separate these into two separate effects, a primary mounting effect and a secondary className change effect.
| return () => removeListener(eventTarget, listener, capture); | ||
| } | ||
| }, [config.isEnabled, config.target, keyCode, callback]); | ||
| }, [config.isEnabled, config.target, config.capture, keyCode, callback]); |
There was a problem hiding this comment.
If the callback is not memoized coming in to this, then we might see a significant amount of churn here. This isn't new to this PR, but something to flag for later.
|
|
||
| useKeydown('Escape', onClose); | ||
|
|
||
| // TODO: For now this doesn't seem to really work due to TLDraw consuming |
There was a problem hiding this comment.
This comment also exists in menu_chart_options.tsx and can be removed from there as well.
| const QUERIES = new Map<EventTarget, Query>(); | ||
| // Capture-phase and bubble-phase listeners require separate `addEventListener` | ||
| // registrations on the same target, so each phase gets its own registry. | ||
| const BUBBLE_QUERIES = new Map<EventTarget, Query>(); |
There was a problem hiding this comment.
Since we are using DOM elements as keys, let's update this to a WeakMap (it shouldn't matter in this particular case, but generally, it will allow a safety-net for garbage collection where destroyed DOM elements can be garbage-collected and removed from the map automatically).
2 participants
Overview
Implemented
Portalelement to fix openMenushowing behind home content.Fixed focus events in
InFrontOfCanvasgetting consumed by tldraw. To do so implemented newcaptureconfig option onuseKeyDown.Re-ordered zoom options:
Tickets:
