Skip to content

Conversation

@ajlende
Copy link
Contributor

@ajlende ajlende commented Jun 29, 2020

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:

  1. Add an image to your media library
  2. Add that new image to a new post
  3. Save as draft
  4. Reload editor
  5. Rotate the placed image in the editor
  6. See that the image rotates properly

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ajlende ajlende added the [Block] Image Affects the Image Block label Jun 29, 2020
@ajlende ajlende requested review from azaozz and ellatrix June 29, 2020 20:19
@ajlende ajlende self-assigned this Jun 29, 2020
@ajlende ajlende force-pushed the fix/crop-preview-react-easy-crop branch from 1a81f1e to b157c96 Compare June 29, 2020 22:29
@ellatrix
Copy link
Member

After rotation, I see this:

Screenshot 2020-06-30 at 15 53 18

@ajlende
Copy link
Contributor Author

ajlende commented Jun 30, 2020

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.

scale crop area

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.

@ellatrix
Copy link
Member

ellatrix commented Jul 1, 2020

Are you referring to the extra space around the crop area?

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.

@ajlende
Copy link
Contributor Author

ajlende commented Jul 1, 2020

and the crop size being off

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?

@ajlende ajlende force-pushed the fix/crop-preview-react-easy-crop branch from b157c96 to e8e31a8 Compare July 1, 2020 15:56
@ellatrix
Copy link
Member

ellatrix commented Jul 1, 2020

If that gets fixed, is the padding/air also a blocker for this PR?

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!

@ajlende
Copy link
Contributor Author

ajlende commented Jul 2, 2020

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!

@ajlende ajlende marked this pull request as draft January 13, 2021 17:06
Base automatically changed from master to trunk March 1, 2021 15:43
@skorasaurus
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Image Affects the Image Block

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error rotating images

3 participants