[PE-56] regression: image aspect ratio fix#5792
Conversation
WalkthroughThe changes made in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (1)
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
219-219: Use Explicit Null Checks When Applying Aspect Ratio StyleIn the style objects on lines 219 and 245, using
(size.aspectRatio && { aspectRatio: size.aspectRatio })may inadvertently skip anaspectRatioof0due to its falsy value in JavaScript. While anaspectRatioof0is invalid, for clarity and to prevent potential bugs, consider using explicit null checks.Apply this diff:
- ...(size.aspectRatio && { aspectRatio: size.aspectRatio }), + ...(size.aspectRatio != null && { aspectRatio: size.aspectRatio }),Also applies to: 245-245
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (5 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
65-65: LGTM on Aspect Ratio InitializationSetting
aspectRatiotoaspectRatio || nullensures that it defaults tonullwhen not provided. This change aligns with the goal of resolving the regression issue where older images had their aspect ratio incorrectly set to1.
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Show resolved
Hide resolved
Palanikannan1437
changed the title
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-block.tsx (1)
107-117: Improved aspect ratio calculation and namingThe changes in this section enhance the accuracy and clarity of aspect ratio calculations:
- Renaming
aspectRatiotoaspectRatioCalculatedclearly distinguishes between the calculated value and the stored attribute.- Using
nodeWidthin the condition is consistent with the earlier renaming.- The calculation of
initialHeightand setting ofaspectRatioininitialComputedSizenow correctly use the calculated aspect ratio.These modifications directly address the PR objective of fixing the aspect ratio regression.
Consider adding a check to prevent division by zero when calculating
aspectRatioCalculated:- const aspectRatioCalculated = img.naturalWidth / img.naturalHeight; + const aspectRatioCalculated = img.naturalHeight !== 0 ? img.naturalWidth / img.naturalHeight : null; + if (aspectRatioCalculated === null) { + console.error("Invalid image dimensions: height is zero"); + return; + }This addition would improve the robustness of the code by handling potential edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (6 hunks)
🧰 Additional context used
🪛 Biome
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
[error] 62-62: Shouldn't redeclare 'remoteImageSrc'. Consider to delete it or rename it.
'remoteImageSrc' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (6)
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (6)
62-67: Improved clarity and handling of image attributesThe changes in destructuring
node.attrsand initializing thesizestate are well-implemented:
- Renaming properties to
nodeWidth,nodeHeight, andnodeAspectRatioimproves code clarity by distinguishing between node attributes and local state.- Setting the default
aspectRatiotonullis more appropriate, as it correctly represents the absence of a value rather than assuming a square aspect ratio.These modifications align well with the PR objective of fixing the aspect ratio regression for older images.
🧰 Tools
🪛 Biome
[error] 62-62: Shouldn't redeclare 'remoteImageSrc'. Consider to delete it or rename it.
'remoteImageSrc' is defined here:
(lint/suspicious/noRedeclare)
Line range hint
127-140: Crucial fix for aspect ratio handling in older imagesThese changes are essential for addressing the aspect ratio regression issue:
- The updated condition ensures that the aspect ratio is correctly set for older images or when there's a mismatch between stored and calculated values.
- Using
aspectRatioCalculatedin thenewSizeobject ensures consistency with the newly calculated aspect ratio.- Adding
nodeAspectRatioto the effect dependencies ensures that the effect runs when the aspect ratio changes, keeping the component reactive to aspect ratio updates.These modifications directly fulfill the PR objective of fixing the aspect ratio regression for older images.
146-149: Improved synchronization between node attributes and component stateThese changes enhance the component's reactivity to size changes:
- Using
nodeWidthandnodeHeightin thesetSizefunction ensures that the component's size state accurately reflects the node's attributes.- Updating the effect dependencies to include
nodeWidthandnodeHeightensures that the effect runs when these attributes change, maintaining proper synchronization.These modifications contribute to the overall goal of correctly handling image dimensions and aspect ratios.
221-221: Improved handling of aspect ratio in container stylingThis change enhances the robustness of the component's styling:
- The conditional spread operator ensures that the
aspectRatiostyle is only applied whensize.aspectRatiois not null.- This prevents potential issues with invalid aspect ratio values being applied to the container, which could lead to unexpected rendering behavior.
This modification aligns well with the earlier changes to handle aspect ratios more carefully and contributes to the overall goal of correctly managing image dimensions.
247-247: Consistent aspect ratio handling for image elementThis change maintains consistency in aspect ratio handling:
- The conditional spread operator ensures that the
aspectRatiostyle is only applied to the image element whensize.aspectRatiois not null.- This approach is consistent with the similar change applied to the container element (line 221).
- It prevents potential styling issues that could arise from applying invalid aspect ratio values to the image.
This modification contributes to the overall robustness of the component's styling and aspect ratio management.
Line range hint
1-292: Overall assessment: Well-implemented fix for image aspect ratio regressionThis pull request effectively addresses the regression issue affecting the aspect ratio of older images. The changes implemented in the
CustomImageBlockcomponent demonstrate a thorough approach to solving the problem:
- Improved clarity in attribute and state management.
- More accurate and robust aspect ratio calculations.
- Proper handling of aspect ratios for both new and existing images.
- Consistent application of aspect ratio styles to both container and image elements.
The code changes are well-structured, maintain consistency, and align perfectly with the PR objectives. The suggested minor improvements (error handling for aspect ratio calculation and removal of a redundant declaration) will further enhance the robustness of the implementation.
Great job on this fix! Once the minor suggestions are addressed, this PR will be ready for merging.
| setEditorContainer, | ||
| } = props; | ||
| const { width, height, aspectRatio } = node.attrs; | ||
| const { src: remoteImageSrc, width: nodeWidth, height: nodeHeight, aspectRatio: nodeAspectRatio } = node.attrs; |
There was a problem hiding this comment.
Remove redundant destructuring of 'remoteImageSrc'
There's a redundant destructuring of 'remoteImageSrc' on this line, which has already been destructured from the props earlier in the component.
To resolve this, please remove 'remoteImageSrc' from the destructuring statement:
- const { src: remoteImageSrc, width: nodeWidth, height: nodeHeight, aspectRatio: nodeAspectRatio } = node.attrs;
+ const { width: nodeWidth, height: nodeHeight, aspectRatio: nodeAspectRatio } = node.attrs;This change will eliminate the redeclaration and improve code clarity.
📝 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.
| const { src: remoteImageSrc, width: nodeWidth, height: nodeHeight, aspectRatio: nodeAspectRatio } = node.attrs; | |
| const { width: nodeWidth, height: nodeHeight, aspectRatio: nodeAspectRatio } = node.attrs; |
🧰 Tools
🪛 Biome
[error] 62-62: Shouldn't redeclare 'remoteImageSrc'. Consider to delete it or rename it.
'remoteImageSrc' is defined here:
(lint/suspicious/noRedeclare)
2 participants
Description
Fixing a regression issues that's causing old image's aspect ratio to be 1.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor