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

(Re-add) spin prop #436

Merged
merged 2 commits into from
Aug 25, 2021
Merged

(Re-add) spin prop #436

merged 2 commits into from
Aug 25, 2021

Conversation

Tenpi
Copy link
Contributor

@Tenpi Tenpi commented Aug 20, 2021

Hi, I apologize for being annoying but I am still having issues using the rotate prop. Here you can see that when the image rotates, the ReactCrop container stays in place so the image bounds are cropped.

2021-08-20-09-29-35

I wasn't able to fix it with CSS, so actually I think that I will stick to the behavior that I had before. I am going to call this prop spin since it's more like you are "spinning" the whole crop selection around.

I know that it might get confusing to distinguish scale/rotate from zoom/spin, but basically...

zoom/spin are corrections made to the cropping bounds whenever a parent element is transformed. scale/rotate are transformations that are applied directly onto the image.

One final note: I had to reset lastYCrossover back to yInversed because it was giving me problems.

@sekoyo
Copy link
Owner

sekoyo commented Aug 20, 2021

Yeah it makes sense for a photo editing type app, I’m good to add the prop will check this out soon

let radians = Math.abs((degrees * Math.PI) / 180.0);
if ((degrees > -45 && degrees <= 45) || (Math.abs(degrees) > 135 && Math.abs(degrees) <= 180)) {
// Top and Bottom
x = scaledX * Math.cos(radians);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the condition it normally enters and there is no difference between the previous output (x = (e.clientX - rect.left) / zoom;) if spin is 0?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge and check later anyway just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Math.cos(0) = 1, when the spin is 0 that will be evaluated as x = scaledX * 1, so there won't be any difference.

<div className="ReactCrop__drag-handle ord-s" data-ord="s" />
<div className="ReactCrop__drag-handle ord-sw" data-ord="sw" />
<div className="ReactCrop__drag-handle ord-w" data-ord="w" />
<div className="ReactCrop__drag-handle ord-nw" data-ord="nw" style={this.getRotatedCursor("nw", spin)} />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to change the inline style more than changing the class name?

Copy link
Contributor Author

@Tenpi Tenpi Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial thought was to change the inline style, but changing the className might also work.

@sekoyo sekoyo merged commit 5c50f99 into sekoyo:master Aug 25, 2021
@sekoyo
Copy link
Owner

sekoyo commented Aug 25, 2021

@Tenpi does this have any implications for you? - 2dcc11b

My thinking is that since the CSS already styles with those cursors it's not necessary to inline style them too? That way there is no change (or change of CSS specificity) for users not using spin

@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 25, 2021

@Tenpi does this have any implications for you? - 2dcc11b

My thinking is that since the CSS already styles with those cursors it's not necessary to inline style them too? That way there is no change (or change of CSS specificity) for users not using spin

No, by the looks of it, it should still work the same. Thanks for fixing a redundancy in my code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants