-
-
Notifications
You must be signed in to change notification settings - Fork 167
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.
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
fix(crop-area): recalculate sizes on media objectFit change #506
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8e9c779:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching this and providing a fix. I have a question though before approving this PR.
@@ -218,6 +220,11 @@ class Cropper extends React.Component<CropperProps, State> { | |||
if (prevProps.video !== this.props.video) { | |||
this.videoRef.current?.load() | |||
} | |||
|
|||
const objectFit = this.getObjectFit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to store this in the state? Couldn't just call this.computeSizes
when this prop changes (like done for other props above)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could not catch this through props since it's not the props.objectFit changes
What is being changed is the result of getObjectFit() method that calculates how to properly align the image inside the container (should horizontal-cover or vertical-cover be applied)
- To determine which one should be applied the image should already be presented on the layout to get its sizes (that's why requires re-render)
- Since we already calculated the sizes and props haven't changed on the second render run, the componentDidUpdate() doesn't call computeSIzes() method
Thats why on getObjectFit() result change we need a callback for computeSizes() and re-render (on state change) to apply new sizes
Thanks again for this PR! |
🚀 PR was released in |
Hello @ValentinH!
I've faced an issue with crop area size computation on picture changes with obejct-fit: "cover".
Pre requirements
Cropper properties
Container css properties:
Steps to reproduce:
(Same for Vertical Big - Horizontal Small image pair)
Expected behaviour
Crop area is 500x500
Observed behaviour
Crop area is 250x250
To fix the issue I've moved the getObjectFIt() result value into state and added callback to recalculateSize() on this state value change.
Issue code example: https://codesandbox.io/p/sandbox/react-easy-crop-demo-forked-6xh2m2?file=%2Fsrc%2Findex.js
Fix code example: https://codesandbox.io/p/sandbox/react-easy-crop-forked-vzrfyk?file=%2Fsrc%2Findex.js