Skip to content
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

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

kruchkou
Copy link
Contributor

@kruchkou kruchkou commented Dec 14, 2023

Hello @ValentinH!

I've faced an issue with crop area size computation on picture changes with obejct-fit: "cover".

Pre requirements

  • Use round cropper with fixed size

Cropper properties

object-fit: "cover",
aspect: 1,
cropShape: "round",

Container css properties:

width: 500px,
height: 500px

Steps to reproduce:

  • Choose Horizontal Big image (all sides > 500px)
  • Change image to Vertical Small (all sides < 500px)

(Same for Vertical Big - Horizontal Small image pair)

Expected behaviour

Crop area is 500x500
Screenshot 2023-12-14 at 15 04 12

Observed behaviour

Crop area is 250x250
Screenshot 2023-12-14 at 14 55 36

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

Copy link

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:

Sandbox Source
react-easy-crop Configuration

Copy link
Owner

@ValentinH ValentinH left a 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();
Copy link
Owner

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)?

Copy link
Contributor Author

@kruchkou kruchkou Dec 14, 2023

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

@ValentinH ValentinH added bug Something isn't working patch Increment the patch version when merged labels Dec 15, 2023
@ValentinH ValentinH merged commit ff0e9dd into ValentinH:main Dec 15, 2023
4 checks passed
@ValentinH
Copy link
Owner

Thanks again for this PR!

Copy link

🚀 PR was released in v5.0.4 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants