feat(alpha-editor): update tooltip layout and organize modifiers #1196
Conversation
|
@dineshsutihar is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve modifications to three components: Changes
Assessment against linked 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
temporarily deployed
to
production
GitHub Actions
Inactive
|
Hi @dineshsutihar Screen.Recording.2024-10-30.at.16.16.16.mov |
Yes, there’s a pre-existing issue with the link feature, and I was planning to create a new issue to address it separately. Additionally, I think a new UI for the link feature would be beneficial. Whenever we click on the link button, it could open a pop-up like this and prompt for a link. What do you think of this design? @John-Paul-Larkin
Could you please create a new issue and assign it to me? I’ll submit a separate PR to fix it. |
![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
🧹 Outside diff range and nitpick comments (7)
components/editor/editor/components/node-selector.tsx (1)
68-68: Enhance dropdown accessibilityGood addition of
aria-expanded! Consider adding complementary ARIA attributes to make the dropdown menu more accessible.Apply these additional attributes to the dropdown section:
- <section className="animate-in fade-in slide-in-from-top-1 fixed top-full z-[99999] mt-1 flex w-48 flex-col overflow-hidden rounded border border-stone-200 bg-white p-1 shadow-xl"> + <section + role="menu" + aria-orientation="vertical" + className="animate-in fade-in slide-in-from-top-1 fixed top-full z-[99999] mt-1 flex w-48 flex-col overflow-hidden rounded border border-stone-200 bg-white p-1 shadow-xl">And to each menu item button:
- className="flex items-center justify-between rounded-sm px-2 py-1 text-sm text-stone-600 hover:bg-stone-100" + role="menuitem" + className="flex items-center justify-between rounded-sm px-2 py-1 text-sm text-stone-600 hover:bg-stone-100"components/editor/editor/components/link-selector.tsx (4)
3-3: Cleanup unused importsThe import statement for
Linkis correct, but the removed imports forCheckandTrashshould be cleaned up from the imports at the top of the file as well.import { cn, getUrlFromString } from "@/utils/utils"; import type { Editor } from "@tiptap/core"; import { Link } from "lucide-react"; -import { Check, Trash } from "lucide-react"; import type { Dispatch, FC, SetStateAction } from "react";
63-67: Consider accessibility improvements for the link buttonWhile the UI has been simplified to just an icon, this might impact accessibility. Consider adding an aria-label to the button.
<Link className={cn("h-4 w-4", { "text-blue-500": editor.isActive("link"), })} + aria-label="Insert or remove link" />
Line range hint
26-58: Review the prompt-based link handlingThe current implementation using
window.promptis not ideal for several reasons:
- Poor user experience compared to modern UI components
- Limited styling and validation capabilities
- No preview or editing features
This should be replaced with a proper UI component as discussed in the PR comments.
Consider implementing a reusable modal component that:
- Provides proper URL validation
- Supports link preview
- Handles protocol addition more elegantly
- Offers better error handling and user feedback
Would you like me to help create a new issue to track this enhancement?
Critical: Link input UI has been replaced with a basic browser prompt
The verification confirms that:
- The commented-out form UI has been replaced with a basic
window.prompt()implementation in thesetLinkfunction- No other components in the codebase handle the link input UI functionality
- This is the only implementation of the link editor functionality, as evidenced by the search results
While the link functionality still works, it has been downgraded from a rich UI form to a basic browser prompt. This change:
- Removes the ability to visually validate URLs
- Eliminates the user-friendly delete button for removing links
- Reduces accessibility and user experience
Recommendation:
- Either revert the commenting out of the form UI until the new implementation is ready
- Or add a TODO comment explaining this is temporary, with a reference to the planned improvement ticket
🔗 Analysis chain
Line range hint
69-108: Critical: Link input UI has been disabled without replacementThe commented-out form represents the UI for link input/editing, but it's currently disabled without a replacement implementation. This creates a suboptimal user experience where users can only input URLs through a basic browser prompt. This aligns with the issue mentioned in the PR comments about the link feature needing improvement.
Based on the PR comments, there's a plan to implement a new pop-up UI for link input. Until the new UI is implemented, consider:
- Either keeping the current form implementation active
- Or adding a TODO comment explaining the temporary state and linking to the follow-up issue
Let's check if there are any other components that might be handling the link input UI:
Let's also check for any editor-related components that might handle links:
And let's verify if there are any other UI components using the editor's link functionality:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential link-related UI components rg -l "link.*input|url.*input" --type tsxLength of output: 71
Script:
#!/bin/bash # Search for potential link-related UI components with ripgrep # Using broader search patterns and without file type restriction rg -l "setLink|unsetLink|link.*input|url.*input" # Also search for potential link-related components by filename fd "link" --extension tsx --extension ts --extension jsx --extension jsLength of output: 340
Script:
#!/bin/bash # Search for editor commands related to links rg "editor.*link" -A 5 -B 5Length of output: 4693
Script:
#!/bin/bash # Search for usage of editor's link attributes or commands rg "getAttributes.*link|setLink|unsetLink" -A 5 -B 5Length of output: 6212
components/editor/editor/components/bubble-menu.tsx (2)
75-84: Consider simplifying the setIsOpen callbackWhile the implementation is correct, the callback could be more concise.
Consider this simplified version:
-setIsOpen={() => { - setIsNodeSelectorOpen(!isNodeSelectorOpen); - setIsLinkSelectorOpen(false); -}} +setIsOpen={(open) => { + setIsNodeSelectorOpen(open); + setIsLinkSelectorOpen(false); +}}This version:
- Uses the state parameter provided by the setter
- Avoids reading the current state unnecessarily
- Maintains the same functionality
Line range hint
75-108: Architecture aligns with tooltip reorganization objectivesThe separation of NodeSelector and LinkSelector components with their conditional rendering and independent state management provides a good foundation for the tooltip layout reorganization. This structure will support:
- Block-level modifiers on the left (via NodeSelector)
- Inline-level modifiers on the right (including LinkSelector)
- Clear separation between modifier types
Consider documenting these architectural decisions in the component's comments to help maintain the design vision during future modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/editor/editor/components/bubble-menu.tsx(3 hunks)components/editor/editor/components/link-selector.tsx(2 hunks)components/editor/editor/components/node-selector.tsx(2 hunks)
🔇 Additional comments (4)
components/editor/editor/components/node-selector.tsx (2)
37-43: Implementation of Text modifier looks good!
The Text item implementation correctly follows tiptap's patterns and aligns with the PR objectives of organizing block-level modifiers.
Let's verify the Text command implementation across the codebase:
✅ Verification successful
Text modifier implementation is consistent with existing usage
The implementation is verified to be correct and consistent with other usages in the codebase:
- The
toggleNode("paragraph", "paragraph")command is used consistently in both the node selector and slash command implementations TextIconis properly scoped to the node selector component- The implementation follows the same pattern as other block-level modifiers in the editor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if there are any other Text command implementations that might conflict
# Expected: No other implementations of paragraph toggle command
ast-grep --pattern 'toggleNode("paragraph", "paragraph")'
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search for paragraph-related commands and Text implementations
# Search for any paragraph node usage in the codebase
# Search for paragraph-related commands
rg -l "paragraph" --type ts --type tsx
# Search for Text-related components and commands
rg -l "TextIcon" --type ts --type tsx
# Search for toggleNode usage
rg "toggleNode" --type ts --type tsx -A 2
Length of output: 202
Script:
#!/bin/bash
# Let's try without specifying file types since tsx is not recognized
# Search for paragraph-related commands and Text implementations
# Search for paragraph node usage
rg "paragraph" -A 2
# Search for TextIcon usage
rg "TextIcon"
# Search for toggleNode usage
rg "toggleNode" -A 2
Length of output: 5785
Line range hint 1-99: Verify alignment with PR objectives
The current implementation includes block-level modifiers but appears to be missing:
- The visual separator mentioned in the PR objectives
- The inline-level modifiers (Bold, Italic, Hyperlink) that should be positioned on the right
Let's check if these are implemented in other components:
components/editor/editor/components/bubble-menu.tsx (2)
27-28: LGTM: Improved error handling for editor commands
The addition of null-safe checks using optional chaining and nullish coalescing operators enhances the robustness of the editor commands.
Also applies to: 33-34
100-108: Verify link feature implementation
Based on the PR comments, there are known issues with the link feature that will be addressed in a separate PR. Ensure this implementation will be compatible with the planned changes to the link UI (pop-up for link input).
Let's check for any existing link-related components or implementations:
✅ Verification successful
Link feature implementation is compatible with future changes
The current implementation in link-selector.tsx is well-structured for future updates:
- It has a commented-out form UI that can be easily enabled for the planned pop-up link input
- The core link functionality (setting/unsetting links, URL validation) is already implemented
- The component accepts
isOpenandsetIsOpenprops needed for the pop-up behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for link-related components and implementations
echo "Searching for link-related files and implementations..."
fd -e tsx -e ts link
rg -i "link.*selector|link.*popup" -t typescript -t tsx
Length of output: 400
Script:
#!/bin/bash
# Let's examine the link-selector implementation and its dependencies
echo "=== Link Selector Implementation ==="
cat components/editor/editor/components/link-selector.tsx
echo -e "\n=== Checking for Link-related UI Components ==="
cat components/ui-components/link.tsx
echo -e "\n=== Checking Custom Link Node Implementation ==="
cat markdoc/nodes/customLink.markdoc.ts
echo -e "\n=== Searching for Link-related Props and Types ==="
rg -A 5 "interface.*Link|type.*Link" --type-add "ts:*.{ts,tsx}"
Length of output: 9532
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
✨ Codu Pull Request 💻
Fixes #1195
Pull Request details
Any Breaking changes
None
Associated Screenshots
[Optional] What gif best describes this PR or how it makes you feel
None