More image editor improvements #4676
Conversation
There was a problem hiding this comment.
We discussed that we'd like that label to follow user's cursor, but our label library (microtip) doesn't support that.
TBH the static version doesn't look good to me. I'd propose to either adjust the library functionality, or use another library.
we can use a small library to make it a proper select
I'm in favor of this option :)
Co-authored-by: Alexander Zaytsev <nqst@users.noreply.github.com>
locale - Flip => Flip horizontally Co-authored-by: Alexander Zaytsev <nqst@users.noreply.github.com>
@nqst, even when you actually use it? I think it looks bad on the screenshot, but in real life it's quite nice (remember we're always starting from 0deg, so it just looks like "the point we started from", quite organic). |
Just tried it once again, but I couldn't change my mind about it. Sorry :) It's not a blocker to merge the current iteration I think, but I believe it would feel much nicer when that particular tooltip was following the handle. P.S. It might be nice to indicate the 0deg point, but in a different way. The most common solution is to add some subtle indication marks to the track, e.g. like here https://m3.material.io/components/sliders/guidelines#0b2644fa-2308-458b-9e8e-f0289d5b483f |
No no, I kind of agree, just made sure you used it real-time too (which gives a slightly better impression than the screenshot version). |
…into `style.scss`
|
@Murderlon would be happy if you test it locally too, alex is reporting no-zoom-in-on-rotation which I'm yet to reproduce locally https://transloadit.slack.com/archives/C0FMW9PSB/p1696582613328759?thread_ts=1695764122.569399&cid=C0FMW9PSB UPDATE: nevermind, managed to reproduce it with Alex's image! |
There was a problem hiding this comment.
Broken rotation is fixed for me, too! Nice 🎉
Here are the other things that we discussed earlier. What do you think about them?
- Removing the zoom in/out buttons
- Restricting the cropping tool so it doesn't go beyond the image (including the rotated image)
- Setting a minimum bounding box area
- Making the tooltip with the rotation value movable
|
Thank you for the update! I’ve managed to almost crash Firefox with a 3.4MB picture — rotating is fine, saving is where it stops responding: Maybe it's just my machine, but I've had similar issues on a Mac before. |
|
It's a 13MB picture! I can reproduce some staggering on save with your image, firefox does stop responding to my hovers and clicks for me for a few milliseconds (and then recovers). The same happens upon me pasting this image into Uppy (again, for a few milliseconds), I think operations with such images are just funamentally computationally expensive (Foliotek/Croppie#324 (comment)). I guess the important questions are:
|
I'm pro this, but I thought we should leave this for further PRs, after we play with it for some time ourselves/refreshen our eyes.
I see a point in limiting the cropbox to the confines of the image, however that would only work nicely if we limit it while the user is moving the cropbox, and we can't do that (hard with cropperjs, we'll get glitches). I think our current version is pretty nice, it both gives us full freedom to crop anything we want AND doesn't let us travel too far off the image.
Meaning minimum cropbox area, right, like this? It can be hard to do for the same reason why it's hard to do the point above, cropperjs doesn't report each event, so we can't just
I described my musings about this here #4715. I'd say the conclusion is - @arturi is justifiably worried about the bundle sizes & thinks materialui is an overkill; so we should implement the tooltip movability manually. I think that's a todo for further PRs. |
No, original is 3.3MB, so the fact that it turns into a 13MB PNG is also bad. Not sure how though, as I just tried again and the resulting JPEG was 1.1MB.
It never did recover, I waited for a minute or two, but some stuff might have been running in the background, plus too many open tabs, it could all contribute to the issue.
No, mostly with the ones |
I thought we could easily do this with minCropBoxWidth and minCropBoxHeight?
Let's do in a follow-up PR, as long as we just flip the default to off, but allow users to re-enable it, I am pro this change.
I agree we should finalize this PR and move on to other things in the next, so it doesn't feel stale and people already benefit from Evgenia’s amazing work here :) |
I took that 13MB screenshot before I edited it in Uppy. Oh, the size is wildly different based on whether I use
Either way - a properly downloaded image is initially 3.1MB, and turns into 1.2MB (the size lessens) after editing, so we don't need to worry about that; and I cannot reproduce the lagging you described with the 3.1MB image at all. |
😅 right, thanks for fresh eyes |
There was a problem hiding this comment.
Just tested this once again. Significant improvement! 👍
Removing zoom in/out buttons
Let's do in a follow-up PR, as long as we just flip the default to off, but allow users to re-enable it, I am pro this change.
Personally, I'd prefer just remove this entirely soon. Destructive zooming seems to be pretty useless and confusing feature to me, to be honest.
Making the tooltip with the rotation value movable
I described my musings about this here #4715.
This issue is now closed, is there a chance we'll get back to it in the near future?
|
Evgenia @lakesare, please carry the todos from here to the next PR if you'll open one 🙏 Very nice work, merging! |
* main: More image editor improvements (#4676) @uppy/react: add useUppyState (#4711) Release: uppy@3.18.1 (#4760) Bump jsonwebtoken from 8.5.1 to 9.0.0 in /packages/@uppy/companion (#4751) Bump react-devtools-core from 4.25.0 to 4.28.4 (#4756) Bump webpack from 5.74.0 to 5.88.2 (#4740) Bump @babel/traverse from 7.22.5 to 7.23.2 (#4739) @uppy/core: fix `sideEffects` declaration (#4759) Release: uppy@3.18.0 (#4754) @uppy/aws-s3-multipart: fix `TypeError` (#4748) Bump tough-cookie from 4.1.2 to 4.1.3 (#4750) example: simplify code by using built-in `throwIfAborted` (#4749) @uppy/aws-s3-multipart: pass `signal` as separate arg for backward compat (#4746) meta: fix TS integration (#4741)
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3 | 3.5.0 | @uppy/provider-views | 3.7.0 | | @uppy/aws-s3-multipart | 3.9.0 | @uppy/react | 3.2.0 | | @uppy/companion | 4.11.0 | @uppy/transloadit | 3.4.0 | | @uppy/companion-client | 3.6.0 | @uppy/tus | 3.4.0 | | @uppy/core | 3.7.0 | @uppy/url | 3.4.0 | | @uppy/dashboard | 3.7.0 | @uppy/utils | 5.6.0 | | @uppy/image-editor | 2.3.0 | @uppy/xhr-upload | 3.5.0 | | @uppy/locales | 3.4.0 | uppy | 3.19.0 | - @uppy/dashboard: Remove uppy-Dashboard-isFixed when uppy.close() is invoked (Artur Paikin / #4775) - @uppy/core,@uppy/dashboard: don't cancel all files when clicking "done" (Mikael Finstad / #4771) - @uppy/utils: refactor to TS (Antoine du Hamel / #4699) - @uppy/locales: locales: add ca_ES (ordago / #4772) - @uppy/companion: Companion+client stability fixes, error handling and retry (Mikael Finstad / #4734) - @uppy/companion: add getBucket metadata argument (Mikael Finstad / #4770) - 7AE7 @uppy/core: simplify types with class generic (JokcyLou / #4761) - @uppy/image-editor: More image editor improvements (Evgenia Karunus / #4676) - @uppy/react: add useUppyState (Merlijn Vos / #4711)
In this PR
readded labels to all buttons
centered slider's label
Notes: we discussed that we'd like that label to follow user's cursor, but our label library (microtip) doesn't support that. I think leaving it in the center is pretty nice though, also gives some orientation to the user as to where the 0° is.
limited cropbox movements
Note 2 different modes of interaction, first you see me moving the cropbox; then you see me resizing the cropbox.
First one is handled by
limitCropboxMovementOnMove, second one is handled bylimitCropboxMovementOnResize.Screen.Recording.2023-09-15.at.13.33.39.mov
change the checkered bg color
Before:
After:
like we discussed in slack with Artur&Alex, made the background theme-dependent
made both
inputrange.scssandcropper.scssveridical copypastes from the library, all our custom styles are now instyle.scssmade granular rotation [-45, 45] instead of [-45, 44] like Alex noted
made slider hover/focus visible in all browsers
Notes