[ENG-412] Node creation popover menu #218
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces a text selection popup feature in the Roam app. It adds new UI components and event listeners to detect text selection and display a contextual menu near the cursor. Supporting utilities manage the popup's rendering and removal, and new styles are applied for the popup's appearance. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant Listeners
participant PopupUtils
participant PopupComponent
User->>Document: Selects text
Document->>Listeners: Fires selectionchange event
Listeners->>PopupUtils: selectionChangeListener invoked
PopupUtils->>PopupUtils: Finds block element & textarea
PopupUtils->>PopupUtils: Calculates cursor position
PopupUtils->>PopupComponent: Renders TextSelectionNodeMenu at cursor
User->>Document: Clicks elsewhere
Document->>Listeners: Fires mousedown event
Listeners->>PopupUtils: documentClickListener invoked
PopupUtils->>PopupUtils: Determines if click outside popup
PopupUtils->>PopupComponent: Removes popup if needed
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![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
🧹 Nitpick comments (6)
apps/roam/src/styles/textSelectionPopup.css (1)
8-14: Prefer using CSS variables or existing Blueprint tokens for colors and radiiHard-coding palette values (
#d3d8de,#f7f9fc, etc.) here makes it harder to keep the extension visually consistent with the host theme and to support dark-mode in the future. Consider exposing them through CSS variables (or referring to Blueprint’s Sass variables) so the popup automatically inherits theme changes.apps/roam/src/utils/initializeObserversAndListeners.ts (1)
269-287: Minor: account for Blueprint v5 class names
bp3-prefixes are Blueprint v3/4. If the library is upgraded, the click-outside test will fail.
Consider a more future-proof test such astarget.closest("[class*='bp']-popover-content")or explicitly includebp4-/bp5-in the selector list.apps/roam/src/utils/renderTextSelectionPopup.tsx (3)
7-8: Type mismatch: container is a DIV, not a SPAN
currentPopupContaineris created withdocument.createElement("div"), so its type should beHTMLDivElementto avoid casting later.-let currentPopupContainer: HTMLSpanElement | null = null; +let currentPopupContainer: HTMLDivElement | null = null;
50-52: Remove leftoverconsole.logto avoid noisy logs in production- const coords = getCoordsFromTextarea(targetTextarea); - console.log(coords); + const coords = getCoordsFromTextarea(targetTextarea);
56-60: Absolute positioning may break inside scrollable/relative containersBecause the popup is inserted inside the block’s parent but positioned
absolute, its offset parent becomes the firstposition: relativeancestor (often the block itself). When the page is scrolled the popup can drift.
Consider usingposition: fixedwith viewport coords, or compute offsets relative todocument.bodyand insert the container there.apps/roam/src/components/DiscourseNodeMenu.tsx (1)
101-130: Escape key no longer closes the menu
keydownListenerintercepts arrows & enter but lost the"Escape"branch that previously dismissed the menu. Users expect Esc to cancel. Add it back:+ } else if (e.key === "Escape") { + onClose(); + document.body.click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/roam/src/components/DiscourseNodeMenu.tsx(8 hunks)apps/roam/src/index.ts(4 hunks)apps/roam/src/styles/textSelectionPopup.css(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(4 hunks)apps/roam/src/utils/renderTextSelectionPopup.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/utils/renderTextSelectionPopup.tsx (3)
findBlockElementFromSelection(9-35)renderTextSelectionPopup(37-74)removeTextSelectionPopup(76-82)
🔇 Additional comments (3)
apps/roam/src/index.ts (2)
108-117: Listener cleanup looks good — double-check capture/bubble symmetry
selectionchangeandmousedownare attached without the{ capture: true }option, but later removed with the default matcher as well – that’s correct.
Just make sure that elsewhere in the codebase we never attach the same listeners withcapture: true, otherwise the removal inunloadwould leave the captured handler dangling.
135-140: Style element already returned → no further actionNice to see the new
textSelectionPopupStyleadded to theelementsarray so Roam takes care of detaching it automatically.apps/roam/src/components/DiscourseNodeMenu.tsx (1)
219-290: Selection preservation works – consider guarding against stale indicesIf the user modifies the textarea before clicking the trigger, the saved
{start,end}indices may be invalid. You could recompute the selection range fromwindow.getSelection()inhandleTriggerMouseDowninstead of relying on initial values.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A few requested changes and comments, but great stuff!
I'm wondering if we couldn't directly call the NodeMenu from the TextSelectionNodeMenu in a way that wouldn't require changes to isOpen, target, and the addition of onInteraction. But this could be a later optimization.
We should also remove the TextSelectionNodeMenu if a user selects text and hits the keyboard hotkey. I think this might be solved with a more direct call as well.
| const keydownListener = useCallback( | ||
| (e: KeyboardEvent) => { | ||
| if (e.key === "ArrowRight" || e.key === "ArrowDown") { | ||
| if (e.key === "ArrowDown") { |
There was a problem hiding this comment.
Any specific reason ArrowRight/ArrowLeft was removed?
There was a problem hiding this comment.
yeah i often select with Shift+ arrow left/right, to select up to specific characters using keyboard-only. adding key listener for ArrowLeft and ArrowRight prevents this flow.
| document.body.click(); | ||
| } else if (e.key === "Escape") { | ||
| onClose(); | ||
| document.body.click(); |
There was a problem hiding this comment.
@trangdoan982 do you remember why document.body.click() was added here? I imagine a user would like to keep editing the block so I want to remove it, but I don't want to re-introduce a bug that this possibly fixing.
There was a problem hiding this comment.
I removed it here: #477
Let me know if that is in error.
There was a problem hiding this comment.
i don't think it was specifically to address any bug but just as general behavior to de-focus. I think it's okay that you remove it
https://www.loom.com/share/ea08a74d1e584f8ab58702b70cf7bf27
Summary by CodeRabbit