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

Improve image rotation #4639

Merged
merged 10 commits into from
Sep 6, 2023
Merged

Improve image rotation #4639

merged 10 commits into from
Sep 6, 2023

Conversation

lakesare
Copy link
Contributor

@lakesare lakesare commented Aug 21, 2023

This PR

Notes

  • I removed requestAnimationFrame code temporarily while developing. Should we add it back, is there some issue/use case that it helps solve that we can observe in action?

Further ideas (notes for myself)

  • make "granular rotation" and "90 deg rotation" work around the cropbox center instead of image center (cropper 1 doesn't support this RotateTo should occur on cropbox center coordinates fengyuanchen/cropperjs#580, but we can do it manually) [decided against it]
  • disable scale-on-rotation if the cropbox doesn't exactly match the original-nonrotated-image-position [decided against it]
  • prohibit cropbox movement to the empty corners
  • on double click - make the cropbox fit the full image again
  • readd labels on hover over buttons
  • on <input type="range"> data-microtip-position should follow the user mouse if possible
  • "Revert everything" button should be on top/more easily discernable from "rotate 90deg"
  • aspect ratio should be a select
  • make rotation [-45, 45] instead of [-44, 45]
  • fix this autoCrop on Image editor (React) does not work. #4666

Screen.Recording.2023-08-25.at.16.36.49.mov

@lakesare lakesare marked this pull request as ready for review August 25, 2023 14:23
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Very nice change and the code LGTM.

What about your notes? Can we create issue(s) for it so we can track it?

@lakesare
Copy link
Contributor Author

lakesare commented Sep 4, 2023

@Murderlon, I will keep working on those after this PR is merged.
I thought it's a good idea to merge image rotation first, because some clients are actively asking for it, and because it's a nice improvement for everyone already.

Copy link
Contributor

@nqst nqst left a comment

Choose a reason for hiding this comment

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

Very nice work!

I have a request: Do you think the tooltip can follow the thumb position, so it's always placed above it?

Screenshot 2023-09-05 at 11 27 38@2x

Another thought: if we already have tooltips, can we show them when hovering the other buttons (Reset, Rotate 90deg, etc)? Think it could make the UI friendlier.

@nqst
Copy link
Contributor

nqst commented Sep 5, 2023

Noticed a minor issue: max clockwise rotation value is 44° in my browser. I expected to see 45° :)

@arturi
Copy link
Contributor

arturi commented Sep 5, 2023 via email

@lakesare
Copy link
Contributor Author

lakesare commented Sep 6, 2023

@nqst

Alex: I have a request: Do you think the tooltip can follow the thumb position, so it's always placed above it?

Yes, also thought that would be nice, the following cryptic #todo describes this behaviour: "on <input type="range"> data-microtip-position should follow the user mouse if possible".

Noticed a minor issue: max clockwise rotation value is 44° in my browser. I expected to see 45° :)

Makes sense, will update to 45° in the next PR.

@arturi arturi merged commit 57336a1 into main Sep 6, 2023
@arturi arturi deleted the lakesare/improve-image-rotation branch September 6, 2023 11:46
@arturi
Copy link
Contributor

arturi commented Sep 6, 2023

Thank you @lakesare! So excited about this. And a shout-out to @tim-kos for the napkin math idea!

github-actions bot added a commit that referenced this pull request Sep 18, 2023
| Package            | Version | Package            | Version |
| ------------------ | ------- | ------------------ | ------- |
| @uppy/companion    |   4.9.0 | @uppy/locales      |   3.3.1 |
| @uppy/compressor   |   1.0.3 | @uppy/tus          |   3.3.0 |
| @uppy/dashboard    |   3.5.3 | uppy               |  3.16.0 |
| @uppy/image-editor |   2.2.0 |                    |         |

- @uppy/tus: Fix: Utilize user-defined onSuccess, onError, and onProgress callbacks in @uppy/tus (choi sung keun / #4674)
- @uppy/dashboard: Make file-editor:cancel event fire when the Image Editor “cancel” button is pressed (Artur Paikin / #4684)
- @uppy/companion: add missing credentialsURL for box (Mikael Finstad / #4681)
- @uppy/companion: remove s3 endpoints if s3 disabled (Mikael Finstad / #4675)
- meta: use latest Node.js version for tests (Antoine du Hamel / #4662)
- meta: Improve Contributing.md (Evgenia Karunus / #4633)
- @uppy/compressor: update file.meta.name after compression, becase format/extension might have changed (Artur Paikin / #4645)
- @uppy/companion: Onedrive refresh tokens (Mikael Finstad / #4655)
- @uppy/companion: catch "invalid initialization vector" instead of crashing (Mikael Finstad / #4661)
- @uppy/image-editor: Improve image rotation (Evgenia Karunus / #4639)
- @uppy/locales: Feature/updating i18n farsi (Parsa Arvaneh / #4638)
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.

Unintuitive image rotation (with images that are not 1:1 ratio)
4 participants