-
Notifications
You must be signed in to change notification settings - Fork 3
ENG-977 Create icon button for canvas drawer #500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENG-977 Create icon button for canvas drawer #500
Conversation
Co-authored-by: mclicks <mclicks@gmail.com>
|
Cursor Agent can help with this pull request. Just |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: mclicks <mclicks@gmail.com>
Co-authored-by: mclicks <mclicks@gmail.com>
Co-authored-by: mclicks <mclicks@gmail.com>
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
- Added CanvasDrawerProvider to manage the canvas drawer's lifecycle and allow it to be called from non-React contexts. - Updated openCanvasDrawer function to utilize the new provider for better state handling. - Refactored CanvasDrawer component to accept unmountRef as a prop. - Enhanced CanvasDrawerButton styling for improved UI consistency.
📝 WalkthroughWalkthroughThis PR introduces external control for the canvas drawer component, enabling it to be triggered from non-React contexts like the command palette. It adds a Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as CanvasDrawerButton
participant Fn as openCanvasDrawer()
participant Provider as CanvasDrawerProvider
participant Overlay as renderOverlay
participant Drawer as CanvasDrawer
User->>UI: Click button
UI->>Fn: Call openCanvasDrawer()
Fn->>Provider: Check provider exists
alt Provider exists
Fn->>Fn: Compute page & shapes
Fn->>Overlay: renderOverlay with CanvasDrawer
Overlay->>Drawer: Mount with unmountRef
Drawer->>User: Display drawer
User->>Drawer: Interact or close
Drawer->>Overlay: Call unmountRef.current()
Overlay->>Provider: Cleanup
else Provider missing
Fn->>UI: Show error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce new external control patterns with context management and error handling, requiring careful verification of the provider lifecycle, unmount flow, and integration points. However, the individual components are straightforward with limited interplay between them. Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/roam/src/components/canvas/CanvasDrawerButton.tsx (1)
7-14: Consider using tldraw's CSS variable directly instead of hardcoding shadow values.The inline
boxShadowduplicates tldraw's--shadow-2variable with hardcoded values. This creates a maintenance burden—if tldraw updates their shadow, this component won't automatically match.Additionally, the
z-index: 250might conflict with other UI elements. Consider coordinating z-index values across components or using a centralized z-index system.Apply this diff to use the CSS variable directly:
<div className="pointer-events-auto absolute top-11 m-2 rounded-lg" style={{ zIndex: 250, - // copying tldraw var(--shadow-2) - boxShadow: - "0px 0px 2px hsl(0, 0%, 0%, 16%), 0px 2px 3px hsl(0, 0%, 0%, 24%), 0px 2px 6px hsl(0, 0%, 0%, 0.1), inset 0px 0px 0px 1px hsl(0, 0%, 100%)", + boxShadow: "var(--shadow-2)", }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/canvas/CanvasDrawer.tsx(5 hunks)apps/roam/src/components/canvas/CanvasDrawerButton.tsx(1 hunks)apps/roam/src/components/canvas/Tldraw.tsx(3 hunks)
🔇 Additional comments (11)
apps/roam/src/components/canvas/CanvasDrawerButton.tsx (2)
1-3: LGTM!The imports are correct and all necessary for the component functionality.
16-21: LGTM!The Button implementation is correct. The
openCanvasDrawerfunction handles errors internally (via toast notifications), so no additional error handling is needed here.apps/roam/src/components/canvas/Tldraw.tsx (3)
87-88: LGTM!The imports are correct and properly structured.
712-712: LGTM!The button placement is correct—it renders after the canvas loads and inside the container where absolute positioning will work properly. The
CanvasDrawerProvider(added at lines 996-998) ensures the drawer context is available when the button is clicked.
996-998: LGTM!The
CanvasDrawerProvidercorrectly wrapsTldrawCanvas, ensuring the drawer context is available for both main window and sidebar renders. This enables the external control pattern introduced inCanvasDrawer.tsx.apps/roam/src/components/canvas/CanvasDrawer.tsx (6)
1-1: LGTM!Added
useRefimport is necessary for the newCanvasDrawerProvidercomponent.
12-12: LGTM!Importing
render as renderToastis necessary for the error notification inopenCanvasDrawer.
51-51: LGTM!Making
pageTitlereactive topageUidviauseMemois the correct approach.
172-190: LGTM!The
handleClosewrapper correctly clearsunmountRef.currentbefore callingonClose(), preventing stale references during unmount.
192-211: LGTM!The provider validation and toggle behavior are implemented correctly. The error handling provides clear feedback to users if the provider is not mounted, and the toggle-off logic properly unmounts and clears the ref.
213-241: LGTM!The data fetching and rendering logic is correct. The function properly extracts shapes from the tldraw store, groups them by UID, and renders the drawer via
renderOverlay. The ref management enables proper cleanup and toggle behavior.
Add an icon button to the TLdraw canvas to open the canvas drawer.
https://www.loom.com/share/7d61087e52d5447cb971830b2fb2ce42