[PE-46] fix: added aspect ratio to resizing#5693
Conversation
|
Warning Rate limit exceeded@Palanikannan1437 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve substantial updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Palanikannan1437
force-pushed
the
fix/image-resize-aspect
branch
from
33bbc45 to
bf10ddc
Compare
Palanikannan1437
force-pushed
the
fix/image-resize-aspect
branch
from
36ceeb4 to
171a2ce
Compare
pushya22
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (14)
packages/editor/src/core/extensions/custom-image/components/toolbar/root.tsx (1)
13-13: LGTM! Consider adding JSDoc for the new property.The addition of the
aspectRatioproperty aligns well with the PR objectives of adding aspect ratio support for image attributes. The type is correctly defined as a number, which is appropriate for aspect ratio values.Consider adding a JSDoc comment to describe the
aspectRatioproperty, its purpose, and any constraints (e.g., positive values only). This would improve code documentation and help other developers understand the property's usage. For example:image: { src: string; height: string; width: string; /** The aspect ratio of the image (width / height). Must be a positive number. */ aspectRatio: number; };packages/editor/src/core/extensions/image/image-component-without-props.tsx (1)
29-31: LGTM! Consider adding documentation foraspectRatiousage.The addition of the
aspectRatioattribute aligns with the PR objectives and enhances the flexibility of image handling. This change eliminates the need to set height on images, as mentioned in the PR summary.Consider adding a brief comment explaining how the
aspectRatioattribute is used and what values it accepts. This would improve the maintainability of the code and help other developers understand its purpose.Example:
aspectRatio: { default: null, // Accepts a string in the format "width:height" (e.g., "16:9") or null for no fixed aspect ratio },packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1)
Line range hint
51-55: Consider adding documentation forfileMapThe
addStoragemethod initializes afileMap, but its purpose is not immediately clear. Consider adding a brief comment explaining the role of thisfileMapin the context of the image extension.packages/editor/src/core/components/editors/editor-container.tsx (1)
Line range hint
21-58: Consider refactoring for improved code organizationWhile the current implementation works well, consider the following suggestions to enhance code organization and maintainability:
Extract the editor focus and paragraph insertion logic into separate functions. This would make the
handleContainerClickfunction more concise and easier to understand.Create a utility function for checking if the current node is within a special block (like lists, tables, etc.). This would reduce code duplication and improve readability.
Consider using more specific error types or including additional error information in the catch block to facilitate easier debugging.
Here's a potential refactoring example:
const isSpecialBlock = (editor: Editor) => { return ( editor.isActive("orderedList") || editor.isActive("bulletList") || editor.isActive("taskItem") || editor.isActive("table") || editor.isActive("blockquote") || editor.isActive("codeBlock") ); }; const focusEditorEnd = (editor: Editor) => { editor.chain().focus("end", { scrollIntoView: false }).run(); }; const insertNewParagraph = (editor: Editor) => { const endPosition = editor.state.doc.content.size; editor.chain().insertContentAt(endPosition, { type: "paragraph" }).run(); editor.chain().setTextSelection(endPosition + 1).run(); }; const handleContainerClick = (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => { if (event.target !== event.currentTarget || !editor || !editor.isEditable) return; try { if (editor.isFocused) return; focusEditorEnd(editor); const { selection } = editor.state; const currentNode = selection.$from.node(); if (currentNode.content.size === 0 && !isSpecialBlock(editor)) { return; } insertNewParagraph(editor); } catch (error) { console.error("Error handling container click:", error); // Consider adding more specific error handling or logging here } };This refactoring improves readability and makes the code more modular and easier to maintain.
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (1)
59-66: LGTM: Refactored handleKeyDown for better key event handlingThe refactoring of the
handleKeyDownfunction is a good improvement. It consolidates the key event handling logic into a single conditional check, making the code more readable and maintainable. The function correctly handles the Escape, +, =, and - keys for various actions, which is consistent with the PR's goal of refining event listener logic.Consider using a switch statement or an object lookup for key handling to improve readability further:
const keyActions = { 'Escape': handleClose, '+': handleIncreaseMagnification, '=': handleIncreaseMagnification, '-': handleDecreaseMagnification }; if (e.key in keyActions) { e.preventDefault(); e.stopPropagation(); keyActions[e.key](); }This approach can make the code more scalable if you need to add more key handlers in the future.
packages/editor/src/core/helpers/editor-commands.ts (2)
129-148: LGTM with suggestions: Enhance robustness and clarity ofinsertImagefunction.The new
insertImagefunction aligns well with the PR objectives, providing flexible image insertion capabilities. However, consider the following improvements:
- Add input validation to prevent potential runtime errors.
- Implement error handling to facilitate debugging.
- Explicitly define the function's return type for clarity.
Here's a suggested refactoring to address these points:
export const insertImage = ({ editor, event, pos, file, range, }: { editor: Editor; event: "insert" | "drop"; pos?: number | null; file?: File; range?: Range; }): boolean => { try { if (!editor) { console.error("Editor is not defined"); return false; } if (range) editor.chain().focus().deleteRange(range).run(); const imageOptions: InsertImageComponentProps = { event }; if (pos !== undefined && pos !== null) imageOptions.pos = pos; if (file) imageOptions.file = file; return editor.chain().focus().insertImageComponent(imageOptions).run(); } catch (error) { console.error("Error inserting image:", error); return false; } };This refactored version includes basic input validation, error handling, and an explicit return type.
Line range hint
1-148: Summary: Changes align well with PR objectives for enhanced image handling.The modifications to this file, particularly the new
insertImagefunction and the updated import, demonstrate a clear focus on improving image handling within the editor. These changes support the PR's goals of enhancing image insertion, including support for different insertion methods (insert, drop) and flexible positioning.The implementation allows for smooth integration of new image handling features, such as aspect ratio support and improved loading states, as mentioned in the PR objectives. However, to fully realize these objectives, ensure that the
InsertImageComponentPropstype and theinsertImageComponentmethod (likely defined elsewhere) incorporate all the new features described in the PR summary.As you continue to develop these features:
- Consider implementing unit tests for the
insertImagefunction to ensure its reliability across different scenarios.- Ensure that the error handling and loading states are properly managed throughout the image insertion process, as mentioned in the PR objectives.
- If not already done, consider documenting the new image insertion process and any configuration options for other developers who might use this functionality.
packages/editor/src/styles/editor.css (4)
Line range hint
1-54: LGTM! Consider adding a default font size class.The addition of
.large-font,.small-font,.sans-serif,.serif, and.monospaceclasses provides excellent flexibility for customizing the editor's typography. The use of CSS custom properties for font sizes and line heights is a great approach for maintaining consistency and enabling easy theming.Consider adding a
.default-fontclass that sets the base font sizes and line heights. This would provide a clear starting point and make it easier to reset to default values if needed.&.default-font { --font-size-h1: 1.5rem; --font-size-h2: 1.25rem; --font-size-h3: 1.125rem; --font-size-h4: 1rem; --font-size-h5: 0.875rem; --font-size-h6: 0.85rem; --font-size-regular: 0.875rem; --font-size-list: var(--font-size-regular); --font-size-code: var(--font-size-regular); --line-height-h1: 2rem; --line-height-h2: 1.75rem; --line-height-h3: 1.5rem; --line-height-h4: 1.375rem; --line-height-h5: 1.25rem; --line-height-h6: 1.25rem; --line-height-regular: 1.375rem; --line-height-list: var(--line-height-regular); --line-height-code: var(--line-height-regular); }
Line range hint
128-140: LGTM! Consider adding a focus style for keyboard navigation.The changes to image styles effectively differentiate between interactive and non-interactive (read-only or loading) images. The hover effect and selection outline for interactive images provide good visual feedback.
To improve accessibility for keyboard users, consider adding a focus style for images. This will help users navigate and interact with images using the keyboard.
&:not(.read-only-image):not(.loading-image) { /* ... existing styles ... */ &:focus-visible { outline: 3px solid rgba(var(--color-primary-100)); filter: brightness(95%); } }
Line range hint
159-245: LGTM! Consider using CSS variables for colors.The updated task list styles provide a polished and customized appearance for checkboxes. The use of pseudo-elements for the check mark is a great approach. The media query for adjusting the layout on smaller screens is a thoughtful addition.
For better consistency and easier theming, consider using CSS variables for colors instead of hardcoded RGB values. This will make it easier to maintain and update the color scheme across the entire component.
Example:
ul[data-type="taskList"] li > label input[type="checkbox"] { /* ... existing styles ... */ border: 1.5px solid rgb(var(--color-text-100)); &:hover { background-color: rgb(var(--color-background-80)); } &:active { background-color: rgb(var(--color-background-90)); } /* ... */ } ul[data-type="taskList"] li[data-checked="true"] > div > p { color: rgb(var(--color-text-400)); /* ... */ }
Line range hint
351-435: LGTM! Consider adding a custom property for font weights.The updated typography styles provide a well-structured and easily customizable system for headings, paragraphs, and lists. The use of CSS variables for font sizes and line heights is excellent for maintaining consistency and enabling easy theming.
To further improve flexibility, consider adding a custom property for font weights. This would allow for easier customization of font weights across different heading levels and text elements.
Example:
.prose { --font-weight-heading: 600; --font-weight-body: 400; :where(h1):not(:where([class~="not-prose"], [class~="not-prose"] *)) { /* ... existing styles ... */ font-weight: var(--font-weight-heading); } /* Apply to other heading levels and body text */ }This change would make it easier to adjust font weights globally or for specific themes without modifying each selector individually.
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
57-79: Simplify conditional rendering for readabilityThe conditional rendering in the
returnstatement (lines 57-79) is complex and may impact readability.Consider extracting the condition into a descriptive variable to clarify the intent:
const showImageBlock = (isUploaded || imageFromFileSystem) && !failedToLoadImage;Then update the JSX:
{showImageBlock ? ( <CustomImageBlock imageFromFileSystem={imageFromFileSystem} editorContainer={editorContainer} editor={editor} failedToLoadImage={failedToLoadImage} getPos={getPos} node={node} setEditorContainer={setEditorContainer} setFailedToLoadImage={setFailedToLoadImage} selected={selected} updateAttributes={updateAttributes} /> ) : ( <CustomImageUploader editor={editor} failedToLoadImage={failedToLoadImage} getPos={getPos} loadImageFromFileSystem={setImageFromFileSystem} node={node} setIsUploaded={setIsUploaded} selected={selected} updateAttributes={updateAttributes} /> )}This enhances readability by clearly expressing the condition's purpose.
packages/editor/src/core/hooks/use-file-upload.ts (1)
36-38: Address the TypeScript error ineditor.commands.uploadImageThere's a
@ts-expect-errorcomment indicating a TypeScript error witheditor.commands.uploadImage. It's important to resolve this to maintain type safety and prevent potential runtime errors.Do you need assistance fixing the TypeScript typings for
editor.commands.uploadImage? I can help with that or open a GitHub issue to track this task.packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
248-254: Accessibility Enhancement for Resize HandleThe resize handle (
<div>at lines 248-254) might not be easily accessible for users relying on keyboard navigation or assistive technologies.Consider adding appropriate
ariaattributes and keyboard event handlers to improve accessibility.<div className={cn( "absolute bottom-0 right-0 translate-y-1/2 translate-x-1/2 size-4 rounded-full bg-custom-primary-100 border-2 border-white cursor-nwse-resize transition-opacity duration-100 ease-in-out", { "opacity-100 pointer-events-auto": isResizing, "opacity-0 pointer-events-none group-hover/image-component:opacity-100 group-hover/image-component:pointer-events-auto": !isResizing, } )} + role="slider" + aria-label="Resize image" + tabIndex={0} onMouseDown={handleResizeStart} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleResizeStart(e); + } + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
- packages/editor/src/core/components/editors/editor-container.tsx (1 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (2 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (2 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (2 hunks)
- packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (6 hunks)
- packages/editor/src/core/extensions/custom-image/components/toolbar/root.tsx (1 hunks)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (5 hunks)
- packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1 hunks)
- packages/editor/src/core/extensions/drop.tsx (4 hunks)
- packages/editor/src/core/extensions/extensions.tsx (1 hunks)
- packages/editor/src/core/extensions/image/image-component-without-props.tsx (2 hunks)
- packages/editor/src/core/extensions/image/image-extension-without-props.tsx (0 hunks)
- packages/editor/src/core/extensions/slash-commands.tsx (2 hunks)
- packages/editor/src/core/helpers/editor-commands.ts (2 hunks)
- packages/editor/src/core/hooks/use-editor.ts (3 hunks)
- packages/editor/src/core/hooks/use-file-upload.ts (4 hunks)
- packages/editor/src/core/plugins/image/image-upload-handler.ts (0 hunks)
- packages/editor/src/core/plugins/image/index.ts (0 hunks)
- packages/editor/src/core/plugins/image/upload-image.ts (0 hunks)
- packages/editor/src/core/plugins/image/utils/index.ts (0 hunks)
- packages/editor/src/core/plugins/image/utils/placeholder.ts (0 hunks)
- packages/editor/src/styles/editor.css (1 hunks)
- web/core/components/editor/rich-text-editor/rich-text-editor.tsx (1 hunks)
- web/core/components/inbox/content/issue-root.tsx (1 hunks)
- web/core/components/issues/issue-detail/main-content.tsx (1 hunks)
- web/core/components/issues/peek-overview/issue-detail.tsx (1 hunks)
💤 Files with no reviewable changes (6)
- packages/editor/src/core/extensions/image/image-extension-without-props.tsx
- packages/editor/src/core/plugins/image/image-upload-handler.ts
- packages/editor/src/core/plugins/image/index.ts
- packages/editor/src/core/plugins/image/upload-image.ts
- packages/editor/src/core/plugins/image/utils/index.ts
- packages/editor/src/core/plugins/image/utils/placeholder.ts
✅ Files skipped from review due to trivial changes (2)
- web/core/components/issues/issue-detail/main-content.tsx
- web/core/components/issues/peek-overview/issue-detail.tsx
🔇 Additional comments (38)
packages/editor/src/core/extensions/custom-image/components/toolbar/root.tsx (1)
13-13: Verify usage of aspectRatio in related componentsWhile the addition of
aspectRatiois correct, it's not directly used in this component. Ensure that:
- The
ImageFullScreenActioncomponent properly utilizes this new property.- Any other components or functions that use the
imageobject are updated to handle the newaspectRatioproperty if necessary.To verify the usage of
aspectRatioin related components, run the following script:This script will help identify where and how the
aspectRatioproperty is being used, allowing us to ensure consistent implementation across related components.packages/editor/src/core/extensions/image/image-component-without-props.tsx (3)
Line range hint
1-53: Summary: Changes enhance image handling, but require verification.The modifications to
CustomImageComponentWithoutPropsalign well with the PR objectives, particularly in adding aspect ratio support and streamlining the component. Key points:
- The new
aspectRatioattribute provides more flexibility in image rendering.- Removal of
ReactNodeViewRendererandaddNodeViewsimplifies the component structure.- The addition of
UploadImageExtensionStoragesuggests improved upload functionality.To ensure these changes don't introduce unintended side effects:
- Verify the usage of
UploadImageExtensionStorage.- Check the impact of removing
addNodeViewon image rendering.- Consider adding documentation for the
aspectRatioattribute.- Update or add unit tests to cover the new behavior.
Overall, these changes appear to enhance the functionality and maintainability of the image component.
4-4: LGTM! Verify the usage of the new import.The import changes align with the removal of the
addNodeViewmethod and the introduction of image upload functionality. The new importUploadImageExtensionStoragesuggests enhanced capabilities for managing image uploads.To ensure the new import is used correctly, please run the following script:
#!/bin/bash # Description: Verify the usage of UploadImageExtensionStorage in the file # Test: Search for UploadImageExtensionStorage usage rg --type typescript "UploadImageExtensionStorage" packages/editor/src/core/extensions/image/image-component-without-props.tsx
Line range hint
1-53: Verify the impact of removingaddNodeViewon image rendering.The removal of the
addNodeViewmethod, as mentioned in the AI summary, aligns with the PR objectives. However, it's important to ensure that this change doesn't negatively impact image rendering in the editor.Please run the following script to check for any remaining references to
addNodeViewand to verify the new image rendering mechanism:Ensure that the image rendering still works as expected after this change. Consider adding unit tests or updating existing ones to cover the new rendering behavior.
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (2)
Line range hint
1-58: Verify implementation of PR objectivesThe PR objectives mention several features such as a smooth blur effect, loading state, and error handling for image uploads. These are not visible in this file. Could you please confirm if these features are implemented in the
CustomImageNodecomponent or elsewhere in the codebase?#!/bin/bash # Search for implementations of blur effect, loading state, and error handling rg --type typescript -e 'blur.*effect' -e 'loading.*state' -e 'error.*handling' packages/editor/src
30-32: LGTM: Addition ofaspectRatioattributeThe addition of the
aspectRatioattribute aligns with the PR objective of adding aspect ratio support for image attributes. Setting the default value tonullprovides flexibility in usage.However, I noticed that the
heightattribute is still present with a default value of "auto". The PR summary mentions eliminating the need to set height on images. Could you clarify if theheightattribute is still necessary?web/core/components/editor/rich-text-editor/rich-text-editor.tsx (1)
54-54: LGTM: Added bottom padding to improve user interaction.The addition of the
pb-3class to the container's className is a good change. This bottom padding aligns with the PR objective of restricting click events on the editor container to user-initiated clicks. It should help prevent accidental interactions near the bottom edge of the editor, improving the overall user experience.packages/editor/src/core/components/editors/editor-container.tsx (2)
21-22: Excellent improvement to click handling!The changes to the
handleContainerClickfunction effectively address the PR objective of restricting click events on the editor container to user-initiated clicks. Here's why this is a great improvement:
- The updated function signature now includes the event parameter, allowing access to crucial event information.
- The new condition
if (event.target !== event.currentTarget) return;ensures that the function only proceeds if the click was directly on the container, preventing unintended behavior when clicking on child elements.These changes enhance the precision of the click handling, improving the overall user experience and reducing potential bugs related to misplaced cursor focus or unintended paragraph insertions.
Line range hint
1-85: Summary of the reviewThe changes made to the
EditorContainercomponent, particularly in thehandleContainerClickfunction, effectively address the PR objectives. The implementation restricts click events to user-initiated clicks on the container, enhancing the precision of editor interactions.Key points from the review:
- The changes are approved and align well with the PR objectives.
- The new event handling improves user experience and reduces potential bugs.
- Suggestions for further refactoring have been provided to enhance code organization and maintainability.
Overall, this is a solid improvement to the editor's functionality. The suggested refactoring, if implemented, would further enhance the code quality without changing the core functionality.
packages/editor/src/core/extensions/extensions.tsx (2)
Line range hint
1-168: Summary of changes in CoreEditorExtensionsThe modification in this file is focused and aligns well with the PR objectives. It updates the way the image upload status is accessed within the Placeholder extension configuration. This change is likely part of a broader refactoring effort to improve image handling across the application.
The isolated nature of this change, with no other modifications to the file, suggests that related changes might be present in other files. It would be beneficial to review these related files to ensure consistency in the new approach to image upload status management.
152-152: Approve the update to image upload status check.The change from
editor.storage.image.uploadInProgresstoeditor.storage.imageComponent.uploadInProgressaligns with the PR objectives of refactoring the image handling process. This update likely reflects a restructuring of how the image upload status is stored in the editor's storage.To ensure this change is consistent across the codebase, please run the following script:
This script will help identify any inconsistencies in how the image upload status is accessed throughout the codebase.
✅ Verification successful
Further Verification Needed on Image Upload Status Usage
The previous script encountered an error due to unrecognized file types. Please run the updated script below to verify the usage of the new image upload status storage path:
Further Verification Needed on Image Upload Status Usage
The previous script failed to recognize the file types. Please run the updated script below to verify the usage of the new image upload status storage path:
Image Upload Status Check Successfully Updated
The search results confirm that the old storage path
editor.storage.image.uploadInProgressis no longer present in the codebase. All usages of the image upload status now consistently referenceeditor.storage.imageComponent.uploadInProgress, aligning with the PR objectives of refactoring the image handling process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new image upload status storage path # Test 1: Check for any remaining instances of the old storage path echo "Checking for old storage path usage:" rg --type typescript --type tsx 'editor\.storage\.image\.uploadInProgress' # Test 2: Verify the usage of the new storage path echo "Checking new storage path usage:" rg --type typescript --type tsx 'editor\.storage\.imageComponent\.uploadInProgress'Length of output: 390
Script:
#!/bin/bash # Description: Verify the usage of the new image upload status storage path # Test 1: Check for any remaining instances of the old storage path echo "Checking for old storage path usage:" rg 'editor\.storage\.image\.uploadInProgress' --type tsx --type ts # Test 2: Verify the usage of the new storage path echo "Checking new storage path usage:" rg 'editor\.storage\.imageComponent\.uploadInProgress' --type tsx --type tsLength of output: 360
Script:
#!/bin/bash # Description: Verify the usage of the new image upload status storage path # Test 1: Check for any remaining instances of the old storage path echo "Checking for old storage path usage:" rg 'editor\.storage\.image\.uploadInProgress' -g '*.ts' -g '*.tsx' # Test 2: Verify the usage of the new storage path echo "Checking new storage path usage:" rg 'editor\.storage\.imageComponent\.uploadInProgress' -g '*.ts' -g '*.tsx'Length of output: 681
web/core/components/inbox/content/issue-root.tsx (1)
130-130: LGTM. Verify visual impact and update PR description.The styling changes to the
IssueDescriptionInputcomponent look good. The removal of!mb-6and addition ofborder-noneseem to be intentional styling adjustments.Please verify the visual impact of these changes:
- Ensure that removing the bottom margin (
!mb-6) doesn't negatively affect the layout or spacing between elements.- Confirm that removing the border (
border-none) aligns with the intended design.Consider updating the PR description to include this styling change, as it's not mentioned in the current objectives.
packages/editor/src/core/components/menus/menu-items.ts (2)
26-26: LGTM: Good modularization practice.The addition of
insertImageto the imports is a positive change. It suggests that this function has been moved to a separate file, which improves code organization and promotes reusability.
197-197: LGTM: Improved modularity, but verify usage.The update to the
ImageItemfunction improves code modularity by delegating the image insertion logic to a separateinsertImagefunction. This change enhances maintainability and readability.Please run the following script to verify that all usages of
ImageItemhave been updated to accommodate this change:This script will help identify any locations where
ImageItemis used and might need updating, as well as any remaining direct uses ofsetImageUploadthat should now useinsertImage.✅ Verification successful
Please run the following updated script to verify that all usages of
ImageItemhave been updated accordingly:
Please run the following updated script to verify that all usages of
ImageItemhave been updated accordingly:
Verified: All usages of
ImageItemare correctly updated.The verification confirms that the
ImageItemfunction is properly updated inmenu-items.tsand there are no outdatedsetImageUploadusages remaining in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of ImageItem that might need updating # Search for ImageItem usages echo "Searching for ImageItem usages:" rg --type typescript --type javascript "ImageItem\(" -C 3 # Search for potential outdated usages of setImageUpload echo "Searching for potential outdated setImageUpload usages:" rg --type typescript --type javascript "setImageUpload.*event:\s*['\"]insert['\"]" -C 3Length of output: 417
Script:
# Description: Check for usages of ImageItem that might need updating # Search for ImageItem usages echo "Searching for ImageItem usages:" rg --type ts --type tsx "ImageItem\(" -C 3 # Search for potential outdated usages of setImageUpload echo "Searching for potential outdated setImageUpload usages:" rg --type ts --type tsx "setImageUpload.*event:\s*['\"]insert['\"]" -C 3Length of output: 373
Script:
# Description: Check for usages of ImageItem that might need updating # Search for ImageItem usages echo "Searching for ImageItem usages:" rg --glob "*.ts" --glob "*.tsx" "ImageItem\(" -C 3 # Search for potential outdated setImageUpload usages echo "Searching for potential outdated setImageUpload usages:" rg --glob "*.ts" --glob "*.tsx" "setImageUpload.*event:\s*['\"]insert['\"]" -C 3Length of output: 775
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (7)
11-11: LGTM: Added aspectRatio property to Props typeThe addition of the
aspectRatioproperty to theimageobject in thePropstype is a good change. It aligns with the PR objective of adding aspect ratio support for image attributes and follows TypeScript best practices by explicitly defining the type for new properties.
21-21: LGTM: Updated props destructuringThe change in destructuring the
imageobject to includeaspectRatioinstead ofheightis consistent with the PR objective. This modification supports the goal of using aspect ratio for image sizing, eliminating the need for explicit height values.
24-25: LGTM: Added modalRef for improved modal managementThe addition of
modalRefusinguseRefis a good practice. This ref will likely be used to manage the modal's DOM reference, which is crucial for implementing features like closing the modal when clicking outside. This change aligns with the PR's goal of enhancing user interaction.
70-78: LGTM: Added handleClickOutside for improved modal interactionThe addition of the
handleClickOutsidefunction is a great improvement to the user experience. It allows users to close the modal by clicking outside of it, which is a common and expected behavior in modern web applications. The function correctly uses themodalRefto check if the click target is the modal backdrop, following React best practices for handling click outside events.
81-86: LGTM: Optimized useEffect for conditional event listenerThe modification of the
useEffecthook is a good optimization. By conditionally adding and removing the keydown event listener based on theisFullScreenEnabledstate, the code ensures that the listener is only active when necessary. This prevents unnecessary event handling when the modal is not open and correctly cleans up the listener when the component unmounts or whenisFullScreenEnabledbecomes false. This change aligns with React best practices for managing side effects and event listeners.
100-100: LGTM: Improved modal interaction and stylingThe changes to the modal container and bottom control bar are good improvements:
- Adding the
onClick={handleClickOutside}andref={modalRef}to the modal container (line 100) implements the click-outside functionality, enhancing user interaction.- The addition of
bg-blackto the bottom control bar (line 117) improves its visibility and the overall user experience.These modifications align well with the PR's objectives of enhancing user interaction and making visual improvements.
Also applies to: 117-117
Line range hint
1-158: Overall assessment: Excellent improvements to the ImageFullScreenAction componentThe changes made to this file significantly enhance the functionality and user experience of the full-screen image viewer component. Key improvements include:
- Addition of aspect ratio support for image sizing
- Refined event handling for keyboard interactions
- Implementation of click-outside functionality for closing the modal
- Optimized event listener management
- Improved styling for better visibility
These changes align well with the PR objectives and follow React and TypeScript best practices. The code is now more maintainable, performant, and user-friendly. Great job on these improvements!
packages/editor/src/core/helpers/editor-commands.ts (1)
7-7: LGTM: Import change aligns with enhanced image handling.The change from
UploadImagetoInsertImageComponentPropsreflects the shift in image insertion management described in the PR objectives. This new import likely provides a more structured approach to handling image insertion properties.packages/editor/src/core/extensions/slash-commands.tsx (1)
230-230: Improved modularity for image insertion logic.The change to use a separate
insertImagefunction enhances code modularity and maintainability. This aligns well with the PR objectives related to image handling improvements.To ensure the correctness of this change, please verify the implementation of the
insertImagefunction in the "@/helpers/editor-commands" file. Specifically, check that it correctly handles therangeparameter to maintain the expected insertion behavior.packages/editor/src/core/extensions/drop.tsx (2)
23-23:⚠️ Potential issueFix incorrect
eventparameter ininsertImagescall withinhandlePasteIn the
handlePastemethod, theeventparameter in theinsertImagesfunction call is set to"drop". Since this is a paste event, it should be"insert"to accurately reflect the type of event, as per the allowed values"insert"or"drop".Apply this diff to fix the event parameter:
- insertImages({ editor: this.editor, files: imageFiles, initialPos: pos, event: "drop" }); + insertImages({ editor: this.editor, files: imageFiles, initialPos: pos, event: "insert" });Likely invalid or redundant comment.
56-56:⚠️ Potential issueRemove unnecessary
asynckeyword frominsertImagesfunctionThe
insertImagesfunction is declared with theasynckeyword, but there are noawaitstatements within its body. Since the function does not perform any asynchronous operations, theasynckeyword can be removed to simplify the code.Apply this diff to remove the
asynckeyword:-const insertImages = async ({ +const insertImages = ({Likely invalid or redundant comment.
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
45-50: Ensure stability ofnode.attrsreference inuseEffectdependencyIn the
useEffecthook (lines 45-50), you're depending onnode.attrs.src. Ifnode.attrsis a new object on each render, this could cause unnecessary re-renders.Check if
node.attrsis a stable reference. If it's recreated on each render, consider restructuring your dependencies:
- Use
node.attrs.srcdirectly in the dependency array.- Ensure that
node.attrsremains the same object if its properties haven't changed.packages/editor/src/core/hooks/use-file-upload.ts (1)
19-19: Verify if direct mutation ofeditor.storage.imageComponent.uploadInProgressis safeDirectly mutating
editor.storage.imageComponent.uploadInProgressmay cause issues ifimageComponentis intended to be immutable or if there are side effects associated with its state changes. It's important to ensure that this mutation is safe and doesn't violate any intended design patterns.To confirm whether it's safe to mutate
uploadInProgressdirectly, you can check if there are setter methods or if the property is meant to be mutable.Run the following script to search for
uploadInProgressusage:This will help determine if direct assignment is acceptable or if you should use a setter method.
✅ Verification successful
Attempting verification with corrected file type.
Direct mutation of
editor.storage.imageComponent.uploadInProgressis safe.After reviewing the definitions and usages of
uploadInProgressacross the codebase, it is clear that direct mutations are intentional and do not violate any design patterns or immutability constraints.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for definitions and usages of `uploadInProgress` in the codebase. # Test: Find how `uploadInProgress` is defined and used. rg --type typescript 'uploadInProgress' -A 5 -B 5Length of output: 84
Script:
#!/bin/bash # Description: Search for definitions and usages of `uploadInProgress` in the codebase using the correct file type. rg --type ts 'uploadInProgress' -A 5 -B 5Length of output: 6436
packages/editor/src/core/extensions/custom-image/custom-image.ts (6)
14-18: Definition ofInsertImageComponentPropsenhances type safety.The new type
InsertImageComponentPropsclearly defines the properties for image insertion, improving code clarity and maintainability.
23-23: Updated command signature utilizesInsertImageComponentPropscorrectly.By adopting the
InsertImageComponentPropstype in theinsertImageComponentcommand, the code gains better type safety and readability.
29-31: FunctiongetImageComponentImageFileMapis implemented appropriately.The use of optional chaining safely handles cases where
imageComponentmight be undefined, preventing potential runtime errors.
63-65: AddedaspectRatioattribute for improved image control.Including the
aspectRatioattribute allows for better handling of image resizing while maintaining the correct proportions.
116-116: Initialization ofuploadInProgressenhances state management.Adding
uploadInProgressto the storage helps in effectively tracking the upload state of images.
Line range hint
122-147:insertImageComponentcommand handles events correctly.The command properly distinguishes between
"insert"and"drop"events, managing thefileMapaccordingly to ensure correct image insertion behavior.packages/editor/src/core/extensions/custom-image/components/image-block.tsx (2)
202-207: Potential Infinite Re-rendering on Image Load ErrorIn the
onErrorhandler for the<img>element, you setfailedToLoadImagestate totrue. However, without proper handling, this might cause the component to re-render indefinitely if the image source is invalid.Check whether setting
failedToLoadImagetriggers a state change that re-attempts to load the image, causing an infinite loop.Consider adding a condition to prevent repeated loading attempts for the same erroneous source.
126-133: Ensure Aspect Ratio Is Defined Before ResizingIn the
handleResizefunction, you check forsize.aspectRatiobefore proceeding. However, there might be cases wheresize.aspectRatioisnull, which would prevent resizing from working correctly.Run the following script to check if
size.aspectRatiocan ever benullduring resize operations:packages/editor/src/core/hooks/use-editor.ts (3)
63-63: Initialization ofsavedSelectionstate variableThe addition of the
savedSelectionstate usinguseState<Selection | null>(null)is appropriate for tracking the editor's selection.
207-207: Ensure consistency inisEditorReadyToDiscardmethodThe
isEditorReadyToDiscardmethod now checkseditorRef.current?.storage.imageComponent.uploadInProgress === false. Verify that this change aligns with the overall upload status tracking and that all dependent logic reflects this update.Run the following script to check for inconsistent references:
#!/bin/bash # Description: Search for old references in the `isEditorReadyToDiscard` implementations. rg 'isEditorReadyToDiscard.*editorRef\.current\?\..*\.storage\.image\.uploadInProgress'
110-110: Verify the correct usage ofimageComponent.uploadInProgressThe change from
editor.storage.image.uploadInProgresstoeditor.storage.imageComponent.uploadInProgressindicates a shift in how image upload status is tracked. Ensure thatimageComponentis the correct storage module and that all other references have been updated accordingly.Run the following script to identify any remaining references to the old property:
And confirm the usage of the new property:
✅ Verification successful
Verification Successful: Correct usage of
imageComponent.uploadInProgressconfirmed. All references to the old property have been updated accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of the old image upload progress property. rg 'editor\.storage\.image\.uploadInProgress'Length of output: 89
Script:
#!/bin/bash # Description: Verify the correct usage of the new imageComponent upload progress property. rg 'editor\.storage\.imageComponent\.uploadInProgress'Length of output: 433
Script:
#!/bin/bash # Description: Re-verify usages of the old image upload progress property. rg 'editor\.storage\.image\.uploadInProgress'Length of output: 47
packages/editor/src/core/extensions/custom-image/components/image-node.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-node.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-node.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Show resolved
Hide resolved
Palanikannan1437
force-pushed
the
fix/image-resize-aspect
branch
from
ffcf23e to
9788599
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
Line range hint
122-160: Approve JSX structure with suggestion for text contentThe component's JSX structure is well-implemented:
- Conditional styling provides clear visual feedback.
- Drag and drop functionality is correctly set up.
- The hidden file input is properly implemented and referenced.
Consider refactoring the text content logic for better readability:
const getDisplayText = () => { if (failedToLoadImage) return "Error loading image"; if (isImageBeingUploaded || existingFile) return "Uploading..."; if (draggedInside) return "Drop image here"; return "Add an image"; }; // In JSX <div className="text-base font-medium"> {getDisplayText()} </div>This refactoring simplifies the nested ternary operators, making the code more readable and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (2 hunks)
🔇 Additional comments (5)
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (5)
1-35: Improved component architecture and responsibilitiesThe changes to imports and props demonstrate a significant architectural improvement:
- The component now handles more of the upload process internally, reducing the burden on parent components.
- The addition of
useCallback,useEffect, anduseMemohooks suggests optimized rendering and side-effect management.- The
nodeprop allows direct interaction with the editor's data model, enabling more precise updates.These changes should lead to better performance and easier maintenance.
79-108: Well-implemented memoization and side effects for file uploadThe use of
useMemofor meta data and existing file, combined with theuseEffecthooks for managing file uploads, is well-implemented:
- Memoization prevents unnecessary recalculations.
- The upload logic efficiently handles both dropped and inserted files.
- The
hasTriggeredFilePickerRefprevents redundant file picker triggers.This implementation should provide a smooth and efficient upload experience.
Line range hint
110-120: Approve onFileChange implementationThe
onFileChangecallback is correctly implemented:
- It properly handles file selection from the input.
- File validity is checked before uploading, preventing invalid files from being processed.
- The
useCallbackdependency array is correct, including only theuploadFilefunction.This implementation ensures efficient and safe file handling.
129-130: Approve error state stylingThe addition of the
failedToLoadImagecondition to the className is a good improvement:
- It provides clear visual feedback when an image fails to load.
- The red color scheme effectively communicates the error state.
- The implementation is consistent with the component's overall styling approach.
This change enhances the user experience by clearly indicating when there's an issue with image loading.
137-137: Approve click handler for improved user interactionThe addition of the onClick handler to the main div is a good improvement:
- It allows the entire component area to trigger file selection.
- This improves user experience by providing a larger clickable area.
- The implementation correctly uses the ref to trigger a click on the hidden file input.
This change makes the component more intuitive and easier to use.
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Outdated
Show resolved
Hide resolved
Palanikannan1437
force-pushed
the
fix/image-resize-aspect
branch
from
241cabe to
709b60c
Compare
| [uploadFile] | ||
| ); | ||
|
|
||
| const getDisplayMessage = useCallback(() => { |
There was a problem hiding this comment.
Not Important: Doesn't look like this requires useCallback since it is just being consumed in the same component
3 participants
Description
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Documentation