fix: adjust crop size when the image size changes #13
Conversation
WalkthroughThe changes update the image cropping functionality by adding dynamic resize support. The implementation introduces a new state variable to track previous image dimensions and a new method to proportionally adjust the crop area when the image size changes. A React hook now monitors image size changes via a ResizeObserver and triggers the resizing logic. Additionally, the image load handler has been updated to ensure that cropping operations are only performed when valid. Changes
Assessment against linked issues
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 (
|
![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
🧹 Nitpick comments (2)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (2)
77-97: LGTM with a minor suggestion!The
resizeCropfunction correctly handles proportional resizing with proper null checks and accurate scaling calculations.Consider adding boundary checks to ensure the resized crop doesn't exceed image dimensions:
const resizedCrop: Crop = { unit: crop.unit, - width: crop.width * scaleX, - height: crop.height * scaleY, - x: crop.x * scaleX, - y: crop.y * scaleY, + width: Math.min(crop.width * scaleX, newWidth), + height: Math.min(crop.height * scaleY, newHeight), + x: Math.min(crop.x * scaleX, newWidth - (crop.width * scaleX)), + y: Math.min(crop.y * scaleY, newHeight - (crop.height * scaleY)), };
102-118: Optimize the useEffect dependencies.The ResizeObserver setup is correct, but including
cropin the dependency array may cause unnecessary observer re-subscriptions when the crop changes.Consider removing
cropfrom the dependencies since the observer setup doesn't depend on it:-}, [crop]); +}, []);The
resizeCropfunction will still have access to the latestcropvalue through the closure, and this change will prevent unnecessary observer re-subscriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-vaadin24
🔇 Additional comments (4)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (4)
22-22: LGTM!The new imports are correctly added and are necessary for implementing the resize functionality.
45-46: LGTM!Good choice using
useRefto track previous image dimensions without triggering re-renders. The type definition is clear and the comment explains its purpose well.
48-75: LGTM!The changes to
onImageLoadare well-structured:
- Good defensive programming with the crop null check
- Correctly initializes prevImgSize for future resize calculations
- Maintains original centering functionality
1-224: Add test coverage for the resize functionality.While the implementation is solid, it would benefit from test coverage to ensure the resize behavior works correctly across different scenarios.
Consider adding tests for:
- Image resize events with different dimensions
- Edge cases (null crops, zero dimensions)
- Maintaining aspect ratio during resize
- Boundary conditions
Would you like me to help generate test cases for these scenarios?
Close #10
This PR adds logic so the crop mask resizes correctly when the image size changes (see preivous behavior in issue description). The logic introduces a new method called resizeCrop to be able to rezise the crop accordingly if needed, taking in consideration previous and current size of the image. A resize observer was added to be able to control this behavior.
Summary by CodeRabbit