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.
There was a problem hiding this comment.
Good spot - updated!
| 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.
There was a problem hiding this comment.
Good spot. In theory with React Compiler functions are memorised so shouldn't be an issue? previously experienced some weird instances where React Compiler didn't behave as expected and lack of memorised callback caused issues. Added ticket to review 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.
There was a problem hiding this comment.
This fix is unrelated the the Menu Chart Options issue here which I fixed in: #399. Will leave as is as the latter PR removes comment alongside the corresponding fix.
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:
