-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Crop preview with react-easy-crop #23569
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
Conversation
1a81f1e to
b157c96
Compare
|
Are you referring to the extra space around the crop area? If so, that's just how react-easy-crop works right now. When rotating, it attempts to resize the crop area based on the calculated width of the rotated image since the image size doesn't change with the rotation. There's two open issues (ValentinH/react-easy-crop#116 and ValentinH/react-easy-crop#141) that are related to rotating that I can tackle if you consider this a blocker. I don't see it as a problem right now, as the extra padding is in line with #22576. |
I'm referring to both the extra padding around the image and the crop size being off. Regarding the extra padding (or "air"), I don't really like this change as it takes away the preview effect. In fact, I think the raster should exactly match the width/height of the image. It could be useful to see the cropped parts of the image, but maybe we can overflow those if the user is going to move the image. |
Hmm, looks like that's only happening with landscape images for me. I'll take a look into it. If that gets fixed, is the padding/air also a blocker for this PR? Or is it something that I could try to fix in react-easy-crop and then fix here with just a dependency update? |
b157c96 to
e8e31a8
Compare
It would be great if we can get it to work without any visual differences from what we have now. Other than that, I think this is a good change! |
|
The fact that you can't zoom out to fill the whole frame is a bug in react-easy-crop. I'm going to fix it there instead of making a workaround here, so this PR will be on hold until that fix gets put into a release. Good catch @ellatrix! |
|
think this can be closed since the original issue #28255 ? Reopen if you think this using react-easy-crop is a better solution long term. |


Description
Replaces the canvas-based rotation preview with react-easy-crop preview.
Fixes #23567. Sometimes rotating the image caused an error due to
canvas.toBlob.It is possible to have a crop out of bounds from the image by using a portrait image, rotating it to landscape and then using a portrait aspect ratio. This may be able to be fixed in a future PR by setting a minimum zoom on rotation and aspect ratio changes, but for now a black background was added so the cropping area is still visible in this condition.
Calculations are done generically for any rotation based on how WP_Image_Editor handles rotations. So this also paves the way for a straighten control (rotate between -45 and 45 degrees) and re-adding the flip option now that react-easy-crop supports flip in version 3.1.0.
How has this been tested?
Extra steps to verify #23567:
Screenshots
Types of changes
Bug fix
Checklist: