Skip to content

[PE-56] regression: image aspect ratio fix #5792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pushya22 merged 3 commits into from
Oct 18, 2024
Merged

[PE-56] regression: image aspect ratio fix #5792

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cfefda
regression: image aspect ratio fix
Palanikannan1437 Oct 10, 2024
5117541
fix: name of variables changed for clarity
Palanikannan1437 Oct 10, 2024
378d407
Merge branch 'preview' into fix/img-aspect-ratio
Palanikannan1437 Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions packages/editor/src/core/extensions/custom-image/components/image-block.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
src: remoteImageSrc,
setEditorContainer,
} = props;
const { width, height, aspectRatio } = node.attrs;
const { src: remoteImageSrc, width: nodeWidth, height: nodeHeight, aspectRatio: nodeAspectRatio } = node.attrs;
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

// states
const [size, setSize] = useState<Size>({
width: ensurePixelString(width, "35%"),
height: ensurePixelString(height, "auto"),
aspectRatio: aspectRatio || 1,
width: ensurePixelString(nodeWidth, "35%"),
height: ensurePixelString(nodeHeight, "auto"),
aspectRatio: nodeAspectRatio || null,
});
const [isResizing, setIsResizing] = useState(false);
const [initialResizeComplete, setInitialResizeComplete] = useState(false);
Expand Down Expand Up @@ -104,17 +104,17 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
}

setEditorContainer(closestEditorContainer);
const aspectRatio = img.naturalWidth / img.naturalHeight;
const aspectRatioCalculated = img.naturalWidth / img.naturalHeight;

if (width === "35%") {
if (nodeWidth === "35%") {
const editorWidth = closestEditorContainer.clientWidth;
const initialWidth = Math.max(editorWidth * 0.35, MIN_SIZE);
const initialHeight = initialWidth / aspectRatio;
const initialHeight = initialWidth / aspectRatioCalculated;

const initialComputedSize = {
width: `${Math.round(initialWidth)}px` satisfies Pixel,
height: `${Math.round(initialHeight)}px` satisfies Pixel,
aspectRatio: aspectRatio,
aspectRatio: aspectRatioCalculated,
};

setSize(initialComputedSize);
Expand All @@ -124,9 +124,10 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
);
} else {
// as the aspect ratio in not stored for old images, we need to update the attrs
if (!aspectRatio) {
// or if aspectRatioCalculated from the image's width and height doesn't match stored aspectRatio then also we'll update the attrs
if (!nodeAspectRatio || nodeAspectRatio !== aspectRatioCalculated) {
setSize((prevSize) => {
const newSize = { ...prevSize, aspectRatio };
const newSize = { ...prevSize, aspectRatio: aspectRatioCalculated };
updateAttributesSafely(
newSize,
"Failed to update attributes while initializing images with width but no aspect ratio:"
Expand All @@ -136,16 +137,16 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
}
}
setInitialResizeComplete(true);
}, [width, updateAttributes, editorContainer, aspectRatio]);
}, [nodeWidth, updateAttributes, editorContainer, nodeAspectRatio]);

// for real time resizing
useLayoutEffect(() => {
setSize((prevSize) => ({
...prevSize,
width: ensurePixelString(width),
height: ensurePixelString(height),
width: ensurePixelString(nodeWidth),
height: ensurePixelString(nodeHeight),
}));
}, [width, height]);
}, [nodeWidth, nodeHeight]);

const handleResize = useCallback(
(e: MouseEvent | TouchEvent) => {
Expand Down Expand Up @@ -217,7 +218,7 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
onMouseDown={handleImageMouseDown}
style={{
width: size.width,
aspectRatio: size.aspectRatio,
...(size.aspectRatio && { aspectRatio: size.aspectRatio }),
}}
>
{showImageLoader && (
Expand All @@ -243,7 +244,7 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
})}
style={{
width: size.width,
aspectRatio: size.aspectRatio,
...(size.aspectRatio && { aspectRatio: size.aspectRatio }),
}}
/>
{showImageUtils && (
Expand Down