ENG-969 Add open node in sidebar button to canvas nodes #495
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
* Introduced opacity and color props to DiscourseContextOverlay for better customization. * Added an "Open in Sidebar" button in DiscourseNodeUtil to improve user interaction. * Updated styles to reflect new props and ensure consistent UI behavior.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds opacity and color props and a discriminated-union prop type to DiscourseContextOverlay, wires those props into icon/text rendering, and adds an "Open in Sidebar" button to canvas nodes while passing opacity/textColor/iconColor from DiscourseNodeUtil into the overlay. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeUI as DiscourseNode (canvas)
participant Overlay as DiscourseContextOverlay
participant Sidebar as Sidebar/OpenBlock
User->>NodeUI: hover / view node
NodeUI->>Overlay: render(props: opacity, textColor, iconColor)
Note right of Overlay: Icons/text rendered\nwith provided color & opacity
User->>NodeUI: click "Open in Sidebar" button
NodeUI->>NodeUI: stopPropagation (click / pointerdown)
NodeUI->>Sidebar: openBlockInSidebar(uid)
Sidebar-->>User: sidebar opens with block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: multiple related type changes and new discriminated-union props, UI behavior additions (button + event handling), and styling propagation across components; requires verifying type safety, event handling edge cases, and consistent styling application. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
523-540: Add an accessible label to the sidebar button.Icon-only controls need an explicit accessible name; a
titlealone won’t be announced by screen readers. Please add anaria-label(or visible text) so the new shortcut remains usable for assistive tech.<Button className="absolute left-1 top-1 z-10" minimal small + aria-label="Open in sidebar" icon={ <Icon icon="panel-stats"apps/roam/src/components/DiscourseContextOverlay.tsx (1)
175-190: Mirror opacity styling on the text spans.Icons use inline opacity, but the text relies on
opacity-${opacity}classes, which can disappear in Tailwind’s purge/JIT path and leave the text at full opacity. Please apply the same inline opacity math to the spans so the visual treatment is consistent and purge‑proof.- <span - className={`mr-1 leading-none opacity-${opacity}`} - style={{ color: textColor }} - > + <span + className="mr-1 leading-none" + style={{ + color: textColor, + opacity: Number(opacity) / 100, + }} + > {loading ? "-" : score} </span> <Icon icon={"link"} color={iconColor} style={{ opacity: `${Number(opacity) / 100}` }} /> - <span - className={`leading-none opacity-${opacity}`} - style={{ color: textColor }} - > + <span + className="leading-none" + style={{ + color: textColor, + opacity: Number(opacity) / 100, + }} + > {loading ? "-" : refs} </span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/DiscourseContextOverlay.tsx(2 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(3 hunks)
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
562-571: Consider making overlay opacity configurable.The overlay position adjustment (line 562) is appropriate. However, the opacity is hardcoded to
"50"(line 568). Consider whether this should be:
- Configurable via discourse node canvas settings
- Matched to the button's opacity for consistency
- Left as-is if 50% is the intended design
If configurability is desired, you could derive it from canvas settings similar to how
coloris handled:const { canvasSettings: { alias = "", "key-image": isKeyImage = "", overlayOpacity = "50" // Add to canvas settings } = {}, } = discourseContext.nodes[shape.type] || {};Then pass it to the overlay:
<DiscourseContextOverlay uid={shape.props.uid} id={`${shape.id}-overlay`} - opacity="50" + opacity={overlayOpacity} textColor={textColor} iconColor={textColor} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/DiscourseContextOverlay.tsx(2 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(3 hunks)
🔇 Additional comments (4)
apps/roam/src/components/DiscourseContextOverlay.tsx (2)
61-84: LGTM! Well-structured opacity type definition.The const array with
as constassertion and derived type usingtypeofis idiomatic TypeScript. This provides both runtime values and compile-time type safety.
86-95: LGTM! Discriminated union correctly enforces tag XOR uid.The type definition ensures either
tagoruidis provided (but not both), which prevents invalid prop combinations at compile time.apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (2)
30-31: LGTM! Correct imports for new functionality.The imports for
openBlockInSidebarand Blueprint components are properly added.
522-540: LGTM! Button implementation follows best practices.The "Open in Sidebar" button correctly:
- Stops event propagation to prevent canvas interactions
- Uses
voidfor the async call since errors are handled byopenBlockInSidebar- Applies semi-transparent styling for subtle UI presence
- Provides a descriptive tooltip
mdroidian
deleted the
eng-969-roam-open-node-button-in-sidebar-for-canvas-nodes
branch
Summary by CodeRabbit
New Features
Style