address two rich text editor bugs #210
Conversation
Southclaws
commented
Oct 12, 2024
•
Southclaws
commented
- file upload input id non-unique so inserts into the wrong editor instance
- menu positioning broken
…ges being uploaded to the wrong editor instance
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/src/components/content/ContentComposer/useContentComposer.ts (3)
9-9: LGTM, but consider removing unused import.The addition of
useIdis appropriate for the newuniqueIDfunctionality. However,useMemois imported but not used in the visible changes.Consider removing the
useMemoimport if it's not needed, or comment on its intended use if it will be used in future changes.
71-71: LGTM: Good use ofdata-editor-idattribute.The addition of the
data-editor-idattribute with theuniqueIDvalue is a good way to uniquely identify each editor instance. This will be helpful for targeting specific editor instances in JavaScript or CSS.For consistency with React naming conventions, consider using camelCase for the data attribute name:
data-editorId.
198-198: LGTM: ExposinguniqueIDin the return object.Adding
uniqueIDto the returned object is a good way to make this value available to consumers of the hook. This can be useful for components that need to reference the specific editor instance.Consider adding a comment or updating the function's JSDoc to document this new return value, explaining its purpose and potential uses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/src/components/content/ContentComposer/ContentComposer.tsx (3 hunks)
- web/src/components/content/ContentComposer/plugins/MenuPlugin.tsx (1 hunks)
- web/src/components/content/ContentComposer/useContentComposer.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (7)
web/src/components/content/ContentComposer/ContentComposer.tsx (5)
35-36: LGTM: Addition ofuniqueIDto destructured propertiesThe inclusion of
uniqueIDin the destructured properties fromuseContentComposeris a good addition. This will allow for unique identification of multipleContentComposerinstances, improving the component's flexibility and reusability.
40-40: LGTM: Unique ID for LStack componentThe addition of
uniqueIDto theidattribute of theLStackcomponent is a good practice. It ensures that each instance of theContentComposerhas a unique identifier for its container, which can be useful for styling, scripting, or accessibility purposes.
Line range hint
200-210: LGTM: Unique IDs for file input and labelThe addition of
uniqueIDto both thehtmlForattribute of the label and theidattribute of the file input is a good practice. This ensures that:
- Each instance of the
ContentComposerhas a unique file input.- The label is correctly associated with its corresponding input, maintaining accessibility.
These changes improve the component's ability to handle multiple instances without ID conflicts.
Line range hint
218-224: LGTM: Unique ID for EditorContent componentThe addition of
uniqueIDto theidattribute of theEditorContentcomponent is a good practice. It ensures that each instance of theContentComposerhas a unique identifier for its editor content, which can be beneficial for:
- Styling specific editor instances
- Scripting or event handling tied to specific editors
- Improved debugging and error reporting
This change aligns well with the overall approach of using unique identifiers throughout the component.
Line range hint
35-224: Overall changes look good, consider verifyinguniqueIDgenerationThe changes to incorporate
uniqueIDthroughout theContentComposercomponent are consistent and well-implemented. They improve the component's ability to handle multiple instances without ID conflicts, which is crucial for larger applications.To ensure the robustness of this implementation:
- Verify that the
uniqueIDis properly generated and passed down from the parent component or generated withinuseContentComposer.- Consider adding a prop type check or default value for
uniqueIDto handle cases where it might not be provided.To verify the
uniqueIDgeneration and usage, you can run the following script:This script will help ensure that
uniqueIDis properly handled throughout the related components.web/src/components/content/ContentComposer/useContentComposer.ts (2)
64-66: LGTM: Good use ofuseId()for unique identification.The addition of
uniqueIDusinguseId()is a good practice for generating stable, unique identifiers. The comment clearly explains its purpose for the menu's file upload input ID.
Line range hint
1-258: Overall assessment: Good improvements for multi-instance support.The changes in this file enhance the
useContentComposerhook by adding unique identification for each editor instance. This is a valuable improvement for scenarios where multiple editors might be used on the same page. The implementation is clean and follows React best practices.Key points:
- Good use of
useId()for generating stable, unique identifiers.- Proper application of the unique ID to the editor's attributes.
- Exposing the
uniqueIDin the hook's return object for flexibility.These changes will facilitate better management and targeting of individual editor instances, improving the overall functionality and maintainability of the component.
| const offsetY = caretY + 16; | ||
| const offsetX = containerLeft; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider aligning the floating menu horizontally with the caret position
Currently, offsetX is set to containerLeft, which positions the floating menu at the left edge of the editor. This may result in the menu appearing far from the caret when it's located towards the right side of the editor, potentially leading to a less intuitive user experience.
To improve usability, consider aligning the menu horizontally with the caret's position by using caretX instead of containerLeft.
Apply the following changes:
const { bottom: caretY } = posToDOMRect(view, from, to);
+const { left: caretX } = posToDOMRect(view, from, to);
const offsetY = caretY + 16;
-const offsetX = containerLeft;
+const offsetX = caretX;
// NOTE: Left is -1px for optical alignment due to the border radius.
this.element.setAttribute(
"style",
- `top: ${offsetY}px; left: ${offsetX - 1}px;`,
+ `top: ${offsetY}px; left: ${offsetX - 1}px;`,
);Also applies to: 121-124
Southclaws
force-pushed
the
fix-rich-text-editor-menu-image-bugs
branch
from
2bf0eed to
6425b52
Compare
