Constrain image when rotated #53
Conversation
|
I've played again with your demo and I actually think we should combine this one and the one you made before where the crop size was changed. I'm not sure where to draw the limit but I think the crop size should only be modified if the image width (or height) gets smaller than the cropper. Another limit that might be easier to define would be to prevent the crop size being bigger than the container ones. What do you think? For example, this is what should the crop size be when rotation is 0: And this is for 90deg rotation: If you want there is also a vertical image (cat.jpeg) in the example folder 😉 |
MattyBalaam
force-pushed
the
rotation-contain-2
branch
from
407ad17
to
56f11c8
Compare
|
I have managed to accomplish this (at least on landscape, I’ll take a look at portrait later when I have some time) by adding an additional check to the |
clean move to restrict helper
MattyBalaam
force-pushed
the
rotation-contain-2
branch
from
56f11c8
to
71843fe
Compare
|
I don't know if you pushed your latest changes, but I actually see the opposite behavior when the image is close to 90deg: it crop size gets even wider. |
|
Apologies, try again now. Did a flawed rebase. |
|
I’ve looked at the cat image now and I’ll need to apply some extra logic to handle the portrait case. |
|
Yes this looks great now! Just one question, do we want to also resize the crop area if the image is zoomed? It's actually not needed but might be simpler: otherwise, if the user zooms, rotate, and zooms back, we need to resize again while zooming back (don't know if this is clear ^^) |
|
I see, so this could be done with an additional check in the crop size calculation which accepts the zoom value perhaps. |
|
I’ve pushed a change purely for handling the sizing on landscape images. I couldn’t get a nice result zooming the crop area. |
There was a problem hiding this comment.
Just a few code style comments but overall this looks great to me! You still need to fix the existing unit tests. Also would you like to add a few test cases for your new functions? You can also add one or 2 end-to-end tests using Cypress if you are interested :)
src/helpers.js
Outdated
| @@ -25,10 +42,12 @@ export function getCropSize(imgWidth, imgHeight, aspect) { | |||
| * @param {number} zoom zoom value | |||
| * @returns {{x: number, y number}} | |||
| */ | |||
| export function restrictPosition(position, imageSize, cropSize, zoom) { | |||
| export function restrictPosition(position, imageSize, cropSize, zoom, rotation) { | |||
There was a problem hiding this comment.
Here you should set the default value for the rotation to 0.
src/helpers.js
Outdated
| @@ -157,3 +176,39 @@ export function getCenter(a, b) { | |||
| y: (b.y + a.y) / 2, | |||
| } | |||
| } | |||
|
|
|||
| function rotate(x, y, xm, ym, a) { | |||
There was a problem hiding this comment.
Could you use more verbose variable names so that it's easier to understand what each of them do? 🙂
src/helpers.js
Outdated
| @@ -157,3 +176,39 @@ export function getCenter(a, b) { | |||
| y: (b.y + a.y) / 2, | |||
| } | |||
| } | |||
|
|
|||
| function rotate(x, y, xm, ym, a) { | |||
| var cos = Math.cos, | |||
There was a problem hiding this comment.
Also please use const here and create a new variable for a (rotationInRadians maybe) to avoid overwriting to function arguments.
There was a problem hiding this comment.
Sure, I was being lazy and had just copied in something I found online, will rewrite to be more descriptive in a couple of days
|
BTW I've just added support for rotation gestures on mobile. It should not have any impact with your PR but let's see. |
|
Hey @mbalaam, just wanted to let you know that I'll be offline for the next 3 weeks starting from tomorrow evening. Thus, if you need this feature soon, we need to wrap this up before :) |
|
Thanks for letting me know, hoping to get some time tomorrow morning on this
…On Tue, 27 Aug 2019, 20:30 Valentin Hervieu, ***@***.***> wrote:
Hey @mbalaam <https://github.com/mbalaam>, just wanted to let you know
that I'll be offline for the next 3 weeks starting from tomorrow evening.
Thus, if you need this feature soon, we need to wrap this up before :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#53>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJQNS3MY3TYYTKGFA4UDMDQGV6GVANCNFSM4IOXSJZQ>
.
|
|
I have updated the docs and unit tests. I’ve not used Cypress and won’t have time to learn it in the next day or so unfortunately. One thing I have noticed is that there will be some floating numbers with a few decimal places. I wondered if it might be a good idea to round these in any helpers that return to a component. |
|
Wonderful! Thank you so much for taking care of this! ❤️ |
|
My pleasure. |
|
Published with v1.15: https://github.com/ricardo-ch/react-easy-crop/releases/tag/v1.15.0 |
Adds support for constraining an image in the crop area when rotated.
See #52