ENG-831 - Refactor tldraw maximize to stop canvas jumping around #423
Conversation
…pdates - Introduced `handleMaximizedChange` to manage fullscreen toggling and DOM manipulation. - Added `updateViewportScreenBounds` for viewport updates using Tldraw's built-in methods. - Updated styles to support maximized state in Tldraw canvas. - Removed unnecessary `maximized` state from props in `createUiOverrides`.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
…raw and uiOverrides components.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactors Tldraw canvas maximize behavior: replaces React state with a DOM-driven toggle, adds viewport bounds synchronization via tldraw Box, updates UI overrides API to a single toggle function, and adds CSS rules to adjust layout when maximized. Container classes/styles are simplified accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI Menu/Command
participant Canvas as TldrawCanvas
participant DOM as DOM Container
participant Viewport as tldraw Viewport
User->>UI: Select "Toggle Full Screen"
UI->>Canvas: toggleMaximized()
rect rgba(200,235,255,0.3)
note right of Canvas: Maximize/unmaximize
Canvas->>DOM: Toggle classes (relative/absolute/inset-0,<br/>.dg-tldraw-maximized)
Canvas->>Viewport: updateViewportScreenBounds(Box.from DOM bounds)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/canvas/Tldraw.tsx(4 hunks)apps/roam/src/components/canvas/tldrawStyles.ts(2 hunks)apps/roam/src/components/canvas/uiOverrides.tsx(2 hunks)
🔇 Additional comments (7)
apps/roam/src/components/canvas/uiOverrides.tsx (2)
424-425: Centralizing fullscreen toggle in a single callback is cleaner.Directly calling toggleMaximized() from the action keeps UI overrides thin and avoids stale state.
320-328: No lingering old maximize API references
Verified viarg -nP '\b(maximized|setMaximized)\b'that only CSS class names include “dg-tldraw-maximized” and nomaximizedorsetMaximizedcalls remain.apps/roam/src/components/canvas/tldrawStyles.ts (2)
3-3: Non-functional CSS annotation LGTM.The /* css */ marker improves editor tooling without changing behavior.
80-85: Scope and lifecycle check for dg-tldraw-maximized.Setting position: static on the article/sidebar wrappers is fine, but if the component unmounts while maximized the class may persist and affect layout. Ensure the class is always removed on unmount, and verify no regressions for sticky elements in those wrappers.
Suggested safety net in the React component cleanup (see Tldraw.tsx comment) and optionally here in the unmount path.
apps/roam/src/components/canvas/Tldraw.tsx (3)
45-46: Importing Box from tldraw is appropriate.This aligns with updateViewportScreenBounds expecting a Box.
422-424: Wiring toggleMaximized through UI overrides is correct.Keeps maximize logic local to the canvas component and avoids accidental editor re-creation.
559-561: Container baseline classes LGTM.Always starting from relative simplifies the toggle logic and reduces diff churn in className.
…ments in requestAnimationFrame for smoother performance.
mdroidian
deleted the
eng-831-focus-point-of-canvas-is-jumping-around
branch
* Refactor Tldraw component to handle maximization state and viewport updates - Introduced `handleMaximizedChange` to manage fullscreen toggling and DOM manipulation. - Added `updateViewportScreenBounds` for viewport updates using Tldraw's built-in methods. - Updated styles to support maximized state in Tldraw canvas. - Removed unnecessary `maximized` state from props in `createUiOverrides`. * - Renamed `setMaximized` to `toggleMaximized` for clarity in both Tldraw and uiOverrides components. * Optimize viewport updates in Tldraw component by wrapping DOM measurements in requestAnimationFrame for smoother performance.
Before:
We were updating state which would re-render tldraw. We also are currently not handling/saving the camera (at the moment).
So this would jump the camera to the original/last save state on minimize or maximize. Very jarring and poor UX.
After:
Instead of state we manipulate the DOM directly. Additionally, we use some of tldraw's built in utilities to to move the camera based on the new viewport. It's not 100% but it is much less jarring. We could spend a bit more time finessing it, but it is now diminishing returns. Sidebar was not fully handled, also diminishing returns.
https://www.loom.com/share/9ff85d293fb14f11af0d7d321b9e6a6e
If we want to do sidebar:
and hope they don't have 2 canvas' open in sidebar.
Summary by CodeRabbit
New Features
Bug Fixes