Scrollwheel support for the image editor #2563
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2563 +/- ##
============================================
- Coverage 68.28% 66.35% -1.93%
============================================
Files 23 689 +666
Lines 1545 11815 +10270
============================================
+ Hits 1055 7840 +6785
- Misses 490 3975 +3485
|
|
Size Change: +27.8 kB (3%) Total Size: 871 kB
ℹ️ View Unchanged
|
| return ( | ||
| <Element> | ||
| <Element ref={editElementRef}> |
There was a problem hiding this comment.
I wonder if the event listener should work on the faded media. Otherwise, the user would always have to scroll within the visibly displayed part of the media only. Thoughts?
There was a problem hiding this comment.
I added this behavior but that required removing point-events: none on the faded image in order to be able to capture the wheel event. This in turn caused the clicks on the faded image to be captured (and thus it broke closing the editor by clicking on the faded part) so I added a click event that clears editing when the faded part is clicked. Please review that behavior to make sure there are no other repercussions from removing pointer-events: none.
| @@ -120,6 +126,7 @@ function MediaEdit({ element, box }) { | |||
| ref: setFullMedia, | |||
| draggable: false, | |||
| opacity: opacity / 100, | |||
| onClick: clearEditing, | |||
There was a problem hiding this comment.
I tried just now and I see it currently already cancels editing when clicked on faded part. Is this needed?
There was a problem hiding this comment.
See comment on @miina 's review, this is now needed because I removed pointer-events: none in order to be able to capture the wheel event on the faded part
There was a problem hiding this comment.
@wassgha This might now conflict with #1192 though now -- based on Sam's comment, the whole image area should be draggable (including the faded part). Thoughts?
| @@ -134,8 +141,35 @@ function MediaEdit({ element, box }) { | |||
|
|
|||
| const isImage = 'image' === type; | |||
| const isVideo = 'video' === type; | |||
|
|
|||
| const wheelHandler = useCallback( | |||
There was a problem hiding this comment.
Why not just inline into useEffect
| setProperties({ scale: newScale }); | ||
| evt.preventDefault(); | ||
| }, | ||
| [setProperties, scale] |
There was a problem hiding this comment.
So, this will be refreshing the callback and by extent rerun useEffect on each event including readding event handlers? I think this could be a lot more efficient. E.g. we can inline wheelHandler inside the useEffect and instead of depending on scale, we can simply use setProperties(function()) that would update scale w/o depending on the current scale value.
swissspidy
added
the
Status: Stale
Summary
Zoom in/out of the image/video using the scroll wheel
User-facing changes
User now has the ability to use the scroll wheel to zoom in/out while editing an image or a video
Testing Instructions
Fixes #1427