[WIKI-505] fix: bubble menu floating ui #7282
Conversation
WalkthroughThis update introduces a modular, plugin-based bubble menu system for a TipTap-based editor. It adds a new BubbleMenu plugin, a React renderer, and a TipTap extension to manage contextual menus. Several bubble menu UI components are refactored to use standardized popover and menu libraries, and the main bubble menu root component is streamlined for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor (TipTap)
participant BubbleMenuPlugin
participant BubbleMenu (React)
participant DOM
User->>Editor (TipTap): Selects text / triggers selection
Editor (TipTap)->>BubbleMenuPlugin: Notifies selection change
BubbleMenuPlugin->>BubbleMenu (React): Requests menu update/show
BubbleMenu (React)->>DOM: Renders/positions bubble menu
User->>BubbleMenu (React): Interacts with menu (e.g., format, link, color)
BubbleMenu (React)->>Editor (TipTap): Executes formatting commands
Poem
✨ Finishing Touches
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 (
|
Palanikannan1437
force-pushed
the
fix/bubble-menu-floating-ui
branch
from
34fc608 to
eb0281b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (1)
32-32: Consider removing unusedisOpenprop from component interface.The
isOpenprop is still declared in the Props type (line 27) but is no longer destructured or used sinceCustomMenumanages its own open state internally.type Props = { editor: Editor; - isOpen: boolean; setIsOpen: Dispatch<SetStateAction<boolean>>; };packages/editor/src/core/components/menus/bubble-menu/bubble-menu-plugin.ts (1)
328-331: Specify delay parameter in setTimeout for clarity.The
setTimeoutcall should explicitly specify the delay parameter for better code clarity.- setTimeout(() => this.update(this.editor.view)); + setTimeout(() => this.update(this.editor.view), 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx(2 hunks)packages/editor/src/core/components/menus/bubble-menu/bubble-menu-plugin.ts(1 hunks)packages/editor/src/core/components/menus/bubble-menu/bubble-menu-renderer.tsx(1 hunks)packages/editor/src/core/components/menus/bubble-menu/bubble-menu.ts(1 hunks)packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx(3 hunks)packages/editor/src/core/components/menus/bubble-menu/link-selector.tsx(2 hunks)packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx(3 hunks)packages/editor/src/core/components/menus/bubble-menu/root.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (1)
Learnt from: vineetk13
PR: makeplane/plane#6391
File: web/styles/react-day-picker.css:249-282
Timestamp: 2025-01-17T05:17:51.953Z
Learning: In the date range picker's CSS, left/right positioning for cell background effects (like in `.rdp-range_start::before`, `.rdp-range_middle::before`, `.rdp-range_end::before`) should use physical properties instead of logical properties, as these create visual effects that should remain consistent regardless of text direction.
🧬 Code Graph Analysis (2)
packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (1)
packages/utils/src/common.ts (1)
cn(8-8)
packages/editor/src/core/components/menus/bubble-menu/bubble-menu.ts (1)
packages/editor/src/core/components/menus/bubble-menu/bubble-menu-plugin.ts (2)
BubbleMenuPluginProps(30-114)BubbleMenuPlugin(531-535)
🔇 Additional comments (12)
packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (2)
60-60: Good styling improvement for vertical alignment.The addition of
items-centerensures proper vertical centering of the alignment buttons, and the padding adjustment provides better visual spacing.
70-70: Excellent transition enhancements for better UX.The addition of transition classes (
transition-all duration-200 ease-in-outfor buttons andtransition-transform duration-200for icons) provides smooth hover and active state animations. The conditional styling for active icons ensures consistent visual feedback.Also applies to: 76-80
packages/editor/src/core/components/menus/bubble-menu/bubble-menu.ts (2)
5-12: Well-designed type definition.The
BubbleMenuOptionstype correctly omitseditorandelementfrom the plugin props while makingelementnullable, which allows for flexible usage patterns where the DOM element might not be immediately available.
30-44: Good defensive programming with conditional plugin registration.The null check for
this.options.elementbefore registering the plugin prevents runtime errors and follows the pattern where extensions should gracefully handle missing dependencies.packages/editor/src/core/components/menus/bubble-menu/bubble-menu-renderer.tsx (2)
17-21: Correct ref forwarding implementation.The ref forwarding logic properly handles both function and object refs, ensuring the created div element is accessible to parent components.
65-65: Good use of portals for bubble menu rendering.Using
createPortalallows the bubble menu to render outside the normal React tree hierarchy, which is appropriate for floating UI elements that need to avoid z-index and overflow issues.packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (2)
28-67: Excellent refactor to use Headless UI Popover.The migration to
Popovercomponent improves code maintainability by removing manual state management and provides better accessibility features out of the box. The button styling and structure are well-organized with proper conditional classes for open states.
68-119: Well-structured color selection panels.The color selection UI is properly organized within
Popover.Panelwith clear sections for text and background colors. The color buttons maintain their functionality while the clear buttons (with Ban icon) provide intuitive color removal options.packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (1)
54-84: Great refactor to use CustomMenu component.The migration to
CustomMenusignificantly reduces boilerplate code while maintaining the existing functionality. The custom button implementation preserves the visual design, and the menu items properly handle commands and state updates with correct event propagation control.packages/editor/src/core/components/menus/bubble-menu/link-selector.tsx (1)
1-116: LGTM! Clean refactoring to use Popover component.The migration to
@headlessui/react's Popover component simplifies state management and follows consistent UI patterns. The URL validation and error handling are properly maintained.packages/editor/src/core/components/menus/bubble-menu/root.tsx (1)
1-179: LGTM! Excellent simplification of the bubble menu component.The refactoring effectively removes manual selection tracking in favor of the plugin-based approach. The
shouldShowlogic is comprehensive, and the component structure is cleaner with better separation of concerns.packages/editor/src/core/components/menus/bubble-menu/bubble-menu-plugin.ts (1)
120-536: Well-architected bubble menu plugin implementation.The plugin demonstrates excellent practices:
- Comprehensive event handling with proper cleanup in
destroy()- Performance optimization through debouncing
- Clean separation between selection tracking and menu positioning
- Proper TypeScript typing throughout
The Floating UI integration provides flexible positioning options, and the special handling for cell selections is a nice touch.
| useEffect(() => { | ||
| const bubbleMenuElement = menuEl.current; | ||
|
|
||
| bubbleMenuElement.style.visibility = "hidden"; | ||
| bubbleMenuElement.style.position = "absolute"; | ||
|
|
||
| if (editor?.isDestroyed) { | ||
| return; | ||
| } | ||
|
|
||
| const attachToEditor = editor; | ||
|
|
||
| if (!attachToEditor) { | ||
| console.warn( | ||
| "BubbleMenu component is not rendered inside of an editor component or does not have editor prop." | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const plugin = BubbleMenuPlugin({ | ||
| updateDelay, | ||
| resizeDelay, | ||
| editor: attachToEditor, | ||
| element: bubbleMenuElement, | ||
| pluginKey, | ||
| shouldShow, | ||
| options, | ||
| }); | ||
|
|
||
| attachToEditor.registerPlugin(plugin); | ||
|
|
||
| return () => { | ||
| attachToEditor.unregisterPlugin(pluginKey); | ||
| window.requestAnimationFrame(() => { | ||
| if (bubbleMenuElement.parentNode) { | ||
| bubbleMenuElement.parentNode.removeChild(bubbleMenuElement); | ||
| } | ||
| }); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [editor]); |
There was a problem hiding this comment.
Fix missing dependencies in useEffect.
The useEffect uses several props (pluginKey, updateDelay, resizeDelay, shouldShow, options) but only includes editor in the dependency array. This could cause stale closures when these props change.
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [editor]);
+ }, [editor, pluginKey, updateDelay, resizeDelay, shouldShow, options]);If options is an object that changes reference on every render, consider memoizing it or restructuring to avoid unnecessary re-registrations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const bubbleMenuElement = menuEl.current; | |
| bubbleMenuElement.style.visibility = "hidden"; | |
| bubbleMenuElement.style.position = "absolute"; | |
| if (editor?.isDestroyed) { | |
| return; | |
| } | |
| const attachToEditor = editor; | |
| if (!attachToEditor) { | |
| console.warn( | |
| "BubbleMenu component is not rendered inside of an editor component or does not have editor prop." | |
| ); | |
| return; | |
| } | |
| const plugin = BubbleMenuPlugin({ | |
| updateDelay, | |
| resizeDelay, | |
| editor: attachToEditor, | |
| element: bubbleMenuElement, | |
| pluginKey, | |
| shouldShow, | |
| options, | |
| }); | |
| attachToEditor.registerPlugin(plugin); | |
| return () => { | |
| attachToEditor.unregisterPlugin(pluginKey); | |
| window.requestAnimationFrame(() => { | |
| if (bubbleMenuElement.parentNode) { | |
| bubbleMenuElement.parentNode.removeChild(bubbleMenuElement); | |
| } | |
| }); | |
| }; | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [editor]); | |
| useEffect(() => { | |
| const bubbleMenuElement = menuEl.current; | |
| bubbleMenuElement.style.visibility = "hidden"; | |
| bubbleMenuElement.style.position = "absolute"; | |
| if (editor?.isDestroyed) { | |
| return; | |
| } | |
| const attachToEditor = editor; | |
| if (!attachToEditor) { | |
| console.warn( | |
| "BubbleMenu component is not rendered inside of an editor component or does not have editor prop." | |
| ); | |
| return; | |
| } | |
| const plugin = BubbleMenuPlugin({ | |
| updateDelay, | |
| resizeDelay, | |
| editor: attachToEditor, | |
| element: bubbleMenuElement, | |
| pluginKey, | |
| shouldShow, | |
| options, | |
| }); | |
| attachToEditor.registerPlugin(plugin); | |
| return () => { | |
| attachToEditor.unregisterPlugin(pluginKey); | |
| window.requestAnimationFrame(() => { | |
| if (bubbleMenuElement.parentNode) { | |
| bubbleMenuElement.parentNode.removeChild(bubbleMenuElement); | |
| } | |
| }); | |
| }; | |
| }, [editor, pluginKey, updateDelay, resizeDelay, shouldShow, options]); |
🤖 Prompt for AI Agents
In
packages/editor/src/core/components/menus/bubble-menu/bubble-menu-renderer.tsx
between lines 23 and 63, the useEffect hook depends on multiple props like
pluginKey, updateDelay, resizeDelay, shouldShow, and options, but only editor is
listed in the dependency array. To fix this, add all these props to the
dependency array to ensure the effect runs correctly when any of them change. If
options is an object that changes reference frequently, memoize it or
restructure the code to prevent unnecessary effect executions and plugin
re-registrations.
| show() { | ||
| if (this.isVisible) { | ||
| return; | ||
| } | ||
|
|
||
| this.element.style.visibility = "visible"; | ||
| this.element.style.opacity = "1"; | ||
| // attach to editor's parent element | ||
| this.view.dom.parentElement?.appendChild(this.element); | ||
|
|
||
| if (this.floatingUIOptions.onShow) { | ||
| this.floatingUIOptions.onShow(); | ||
| } | ||
|
|
||
| this.isVisible = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check before DOM manipulation.
The element might not be appended if parentElement is null. Consider adding a fallback or explicit check.
show() {
if (this.isVisible) {
return;
}
this.element.style.visibility = "visible";
this.element.style.opacity = "1";
- // attach to editor's parent element
- this.view.dom.parentElement?.appendChild(this.element);
+ // attach to editor's parent element
+ const parentElement = this.view.dom.parentElement;
+ if (parentElement) {
+ parentElement.appendChild(this.element);
+ } else {
+ console.warn("Unable to attach bubble menu: editor parent element not found");
+ }
if (this.floatingUIOptions.onShow) {
this.floatingUIOptions.onShow();
}
this.isVisible = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| show() { | |
| if (this.isVisible) { | |
| return; | |
| } | |
| this.element.style.visibility = "visible"; | |
| this.element.style.opacity = "1"; | |
| // attach to editor's parent element | |
| this.view.dom.parentElement?.appendChild(this.element); | |
| if (this.floatingUIOptions.onShow) { | |
| this.floatingUIOptions.onShow(); | |
| } | |
| this.isVisible = true; | |
| } | |
| show() { | |
| if (this.isVisible) { | |
| return; | |
| } | |
| this.element.style.visibility = "visible"; | |
| this.element.style.opacity = "1"; | |
| // attach to editor's parent element | |
| const parentElement = this.view.dom.parentElement; | |
| if (parentElement) { | |
| parentElement.appendChild(this.element); | |
| } else { | |
| console.warn("Unable to attach bubble menu: editor parent element not found"); | |
| } | |
| if (this.floatingUIOptions.onShow) { | |
| this.floatingUIOptions.onShow(); | |
| } | |
| this.isVisible = true; | |
| } |
🤖 Prompt for AI Agents
In packages/editor/src/core/components/menus/bubble-menu/bubble-menu-plugin.ts
around lines 481 to 496, the code appends this.element to
this.view.dom.parentElement without confirming parentElement is not null. Add an
explicit null check for this.view.dom.parentElement before calling appendChild
to avoid errors when parentElement is null. If parentElement is null, skip the
appendChild call or handle the fallback appropriately.
| function combineDOMRects(rect1: DOMRect, rect2: DOMRect): DOMRect { | ||
| const top = Math.min(rect1.top, rect2.top); | ||
| const bottom = Math.max(rect1.bottom, rect2.bottom); | ||
| const left = Math.min(rect1.left, rect2.left); | ||
| const right = Math.max(rect1.right, rect2.right); | ||
| const width = right - left; | ||
| const height = bottom - top; | ||
| const x = left; | ||
| const y = top; | ||
| return new DOMRect(x, y, width, height); | ||
| } |
There was a problem hiding this comment.
Check browser compatibility for DOMRect constructor.
The DOMRect constructor is not supported in older browsers (e.g., IE, older versions of Safari). Consider using a polyfill or creating an object that mimics DOMRect.
Apply this diff for better browser compatibility:
-function combineDOMRects(rect1: DOMRect, rect2: DOMRect): DOMRect {
+function combineDOMRects(rect1: DOMRect, rect2: DOMRect): DOMRect {
const top = Math.min(rect1.top, rect2.top);
const bottom = Math.max(rect1.bottom, rect2.bottom);
const left = Math.min(rect1.left, rect2.left);
const right = Math.max(rect1.right, rect2.right);
const width = right - left;
const height = bottom - top;
const x = left;
const y = top;
- return new DOMRect(x, y, width, height);
+
+ // For better browser compatibility
+ const rect = {
+ x,
+ y,
+ width,
+ height,
+ top,
+ right,
+ bottom,
+ left,
+ toJSON() {
+ return { x, y, width, height, top, right, bottom, left };
+ }
+ };
+
+ return rect as DOMRect;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function combineDOMRects(rect1: DOMRect, rect2: DOMRect): DOMRect { | |
| const top = Math.min(rect1.top, rect2.top); | |
| const bottom = Math.max(rect1.bottom, rect2.bottom); | |
| const left = Math.min(rect1.left, rect2.left); | |
| const right = Math.max(rect1.right, rect2.right); | |
| const width = right - left; | |
| const height = bottom - top; | |
| const x = left; | |
| const y = top; | |
| return new DOMRect(x, y, width, height); | |
| } | |
| function combineDOMRects(rect1: DOMRect, rect2: DOMRect): DOMRect { | |
| const top = Math.min(rect1.top, rect2.top); | |
| const bottom = Math.max(rect1.bottom, rect2.bottom); | |
| const left = Math.min(rect1.left, rect2.left); | |
| const right = Math.max(rect1.right, rect2.right); | |
| const width = right - left; | |
| const height = bottom - top; | |
| const x = left; | |
| const y = top; | |
| // For better browser compatibility | |
| const rect = { | |
| x, | |
| y, | |
| width, | |
| height, | |
| top, | |
| right, | |
| bottom, | |
| left, | |
| toJSON() { | |
| return { x, y, width, height, top, right, bottom, left }; | |
| } | |
| }; | |
| return rect as DOMRect; | |
| } |
🤖 Prompt for AI Agents
In packages/editor/src/core/components/menus/bubble-menu/bubble-menu-plugin.ts
around lines 18 to 28, the code uses the DOMRect constructor which lacks support
in older browsers like IE and some Safari versions. To fix this, replace the
DOMRect constructor with an object literal that mimics the DOMRect properties
(x, y, width, height, top, bottom, left, right) to ensure compatibility across
all browsers without relying on the constructor.
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
Description
Added floating UI in Bubble menu, with proper positioning while selecting
Summary by CodeRabbit
New Features
Refactor
Style