Uh oh!
There was an error while loading. Please reload this page.
feat(alpha-editor): update tooltip icons and layout in alpha editor - #1193
feat(alpha-editor): update tooltip icons and layout in alpha editor#1193dineshsutihar wants to merge 2 commits into
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 ( |
@dineshsutihar is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
components/editor/editor/components/link-selector.tsx (1)
63-67: Consider adding aria-label for accessibility
The icon implementation looks good and matches the PR objectives. However, since we removed the text label, we should ensure screen reader accessibility.
<button
type="button"
className="flex h-full items-center space-x-2 px-3 py-1.5 text-sm font-medium text-stone-600 hover:bg-stone-100 active:bg-stone-200"
+ aria-label="Insert link"
onClick={setLink}
>components/editor/editor/components/bubble-menu.tsx (1)
28-39: LGTM: New menu items match requirements.
The implementation correctly adds all required menu items with proper editor commands and null safety checks.
Consider grouping related menu items together (e.g., text formatting vs block-level items) for better code organization:
const items: BubbleMenuItem[] = [
// Text style items
{
name: "text",
// ... text item implementation
},
{
name: "heading",
// ... heading item implementation
},
{
name: "bold",
// ... bold item implementation
},
// ... other text formatting items
// Block-level items
{
name: "quote",
// ... quote item implementation
},
{
name: "horizontalRule",
// ... horizontalRule item implementation
},
];Also applies to: 58-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/editor/editor/components/bubble-menu.tsx (3 hunks)
- components/editor/editor/components/link-selector.tsx (3 hunks)
🔇 Additional comments (4)
components/editor/editor/components/link-selector.tsx (2)
3-3: LGTM: Clean import changes
The import changes align with the PR objectives, properly replacing the old icon imports with the new Link2Icon.
Line range hint 69-105: Remove commented code
There's a large block of commented-out code that appears to be a form UI for link input. If this functionality has been moved elsewhere or is no longer needed, the commented code should be removed. If it's intended for future use, consider:
- Moving it to a separate component for future reference
- Creating a GitHub issue to track the pending work
- Adding a TODO comment with the issue reference
Let's verify if this form UI functionality exists elsewhere:
components/editor/editor/components/bubble-menu.tsx (2)
9-12: LGTM: Icon imports align with requirements.
The new icon imports correctly implement the requirements from issue #1129 for adding quote, text, and heading icons.
Line range hint 75-86: Verify the impact of removing heading visibility check.
The bubble menu visibility logic has been simplified to only check for image selection. Please confirm this change doesn't affect the editor's usability when working with headings.
✅ Verification successful
The previous command failed due to typescript file type issue. Let's try a broader search to find heading-related logic and bubble menu implementations to verify the impact of this change.
Removing heading check from bubble menu visibility is safe
The removal of the heading visibility check from the bubble menu is safe because:
- Heading-related
isActivechecks are properly handled in other components:node-selector.tsxmanages heading level selection (h2, h3)disable-heading-text-style-shortcuts.tsxhandles text styling shortcuts specifically for headings
- The bubble menu's original heading check was redundant since heading-specific interactions are managed by these dedicated components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash# Search for any related heading visibility logic in other editor components
rg "isActive\(\"heading\"\)" --type typescript
Length of output: 81
Script:
#!/bin/bash# Search for any related heading visibility logic using ripgrep without file type restriction
rg "isActive.*heading"# Also search for heading-related code patterns using ast-grep
ast-grep --pattern 'isActive($expr)'# Search for bubble menu related files
fd "bubble.*menu" --type f
Length of output: 1546
Uh oh!
There was an error while loading. Please reload this page.
| isActive: () => props.editor?.isActive("bold") ?? false, | ||
| command: () => props.editor?.chain().focus().toggleBold().run(), |
There was a problem hiding this comment.
💡 Codebase verification
Error handling is inconsistent across editor operations
The search results reveal inconsistencies in error handling:
- In
bubble-menu.tsx, allprops.editoraccesses use optional chaining (?.) and null coalescing (??) - However, in other files:
slash-command.tsxuses direct access:editor: props.editornode-selector.tsxuses directeditoraccess without optional chaining in its commandslink-selector.tsxalso uses directeditoraccess without safeguards
These inconsistencies could lead to runtime errors if the editor instance is undefined.
🔗 Analysis chain
Verify error handling consistency across all editor operations.
While the optional chaining is correctly implemented for these operations, let's ensure it's consistently applied throughout the codebase.
Also applies to: 48-49, 54-55
The previous command failed because the file type was not recognized. Let's try with explicit file extensions and search for both the editor access patterns and chain operations to verify the consistency of error handling.
Let's try without file type flags since they're not recognized, and also look for the specific file we're reviewing to understand its location and potentially related files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash# Search for direct editor access without optional chaining
rg "props\.editor\." --type typescript
Length of output: 75
Script:
#!/bin/bash# Search for editor access patterns in TypeScript/TSX files
rg "props\.editor" -t tsx -t ts
# Also search for chain().focus() patterns to find editor operations
rg "chain\(\)\.focus\(\)" -t tsx -t ts
Length of output: 130
Script:
#!/bin/bash# Search for editor access patterns without file type flags
rg "props\.editor"# Search for chain().focus() patterns
rg "chain\(\)\.focus\(\)"# List all files that might contain editor operations
fd "bubble-menu" --extension tsx --extension ts
Length of output: 4878
There was a problem hiding this comment.
I've updated the error handling in bubble-menu.tsx to use optional chaining and null coalescing as suggested, since TypeScript was indicating that props.editor could potentially be null. This change provides better type safety while maintaining the component's functionality.
There was a problem hiding this comment.
@dineshsutihar, great job updating the error handling in bubble-menu.tsx with optional chaining and null coalescing. This enhances type safety and code robustness.
There are similar instances in other files (slash-command.tsx, node-selector.tsx, link-selector.tsx) where props.editor is accessed directly without optional chaining. Updating these would further improve consistency and prevent potential runtime errors.
(^_^)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/editor/editor/components/bubble-menu.tsx (1)
28-69: Consider extracting editor commands to a shared utility.
The menu items array contains repeated patterns of editor chain commands. Consider extracting these into a shared utility to:
- Reduce code duplication
- Centralize error handling
- Make command patterns more maintainable
Example structure:
consteditorCommands={toggleHeading: (editor: Editor,level: number)=>editor?.chain().focus().toggleHeading({ level }).run(),setParagraph: (editor: Editor)=>editor?.chain().focus().setParagraph().run(),// ... other commands};📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/editor/editor/components/bubble-menu.tsx (3 hunks)
🔇 Additional comments (4)
components/editor/editor/components/bubble-menu.tsx (4)
9-12: LGTM: New icon imports align with PR objectives.
The added imports support the requested tooltip modifications, including new icons for quotes, horizontal rules, text, and headings.
28-39: LGTM: New menu items are well-implemented.
The new menu items for heading, text, quote, and horizontal rule:
- Follow consistent implementation patterns
- Include proper error handling with optional chaining
- Align with the PR objectives to update tooltip icons and layout
Also applies to: 58-69
120-128: LGTM: Improved LinkSelector implementation.
The LinkSelector implementation:
- Adds proper null checking for the editor prop
- Maintains clean state management with a single toggle
75-75: Verify the simplified bubble menu visibility logic.
The condition for showing the bubble menu has been simplified to only check for active images. Please verify that this change doesn't affect the editor's usability in other scenarios, particularly with headings which were previously handled.
✅ Verification successful
Bubble menu visibility logic is correctly implemented
The current implementation is actually correct and doesn't need modification. Here's why:
- The bubble menu hides when an image is active because images have their own dedicated resizer component (
MediaResizer) - Headings are still properly handled through the
itemsarray in the bubble menu, where heading-related actions are available when text is selected - The
shouldShowlogic specifically excludes images to prevent conflict with the image resizer, while allowing the menu to appear for all other valid selection scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash# Search for previous implementations of shouldShow to understand the changes
rg -B 5 -A 5 "shouldShow.*editor\.isActive"Length of output: 45
Script:
#!/bin/bash# Let's try a broader search for bubble menu visibility logic
rg -B 5 -A 5 "isActive.*image"# Also search for any bubble menu related code
rg -B 5 -A 5 "BubbleMenu"# And look for heading-related checks
rg -B 5 -A 5 "isActive.*heading"Length of output: 13214
John-Paul-Larkin
commented
Oct 30, 2024
Apologies dineshsutihar. There has been miscommunication and the details in the issue are incorrect. Expected behaviour for this tooltip is as follows: A dividing line should be in the center. Inline level modifiers should be to the right, to include: Note the Horizontal rule is not required on the tooltip. I am very sorry for this. Would you like to continue with this issue? |
dineshsutihar
commented
Oct 30, 2024
No problem, I can handle those changes. To keep everything organized, could we close this issue and create a new one with the updated details, then assign it to me? I’m already working on it. I’ll close the current PR, as it’s become a bit cluttered, and start a fresh one. Let me know if this approach works for you. @John-Paul-Larkin |
John-Paul-Larkin
commented
Oct 30, 2024
Thanks for being so understanding about this. I will close this PR and create a new issue. |
John-Paul-Larkin
commented
Oct 30, 2024
@dineshsutihar |
✨ Codu Pull Request 💻
Fixes#1129
Pull Request details
Any Breaking changes
None
Associated Screenshots
[Optional] What gif best describes this PR or how it makes you feel
None