search

Foliotek / Croppie

A Javascript Image Cropper
http://foliotek.github.io/Croppie
MIT License
2.58k stars 883 forks source link

Keyboard zoom functionality broken after zoomer.step change #184

Open ablythe opened 8 years ago

ablythe commented 8 years ago

After #142, the zoom keyboard functionality does not work due to the step size being so small. This includes when you are focused on the viewport and trying to use the SHIFT + UP/DOWN key and when you are focused on the zoom itself and using the UP/DOWN keys.

If I'm understanding correctly, it looks like the step change was made to make zooming with the zoomer smoother. By my eye, the change in smoothness isn't that noticeable between 0.01 and 0.0001, so perhaps it makes sense to just revert the change so as not to break the keyboard functionality?

I also tried using 0.001 and while the keyboard zoom works, it's extremely slow.

thedustinsmith commented 8 years ago

I think the zoom step makes a bigger impact when cropping large images.

Maybe it makes more sense for the step to be based on the ratio of the size of the viewport to the size of the image. Although I don't know what that math looks like right now.

thedustinsmith commented 8 years ago

In addition to making the step based on image-to-viewport size difference, I think the keyboard zoom should probably have some sort of multiplier for the step. You're right that .0001 is way too slow for keyboard movement.

ablythe commented 8 years ago

Agreed that a zoom that changes based on image to viewport size difference could make a lot of sense here.

The multiplier might also be a good interim fix. Adding it to https://github.com/Foliotek/Croppie/blob/master/croppie.js#L618-L621 for the zooming when focused on the viewport wouldn't be difficult.

Adding the multiplier on the zoomer keyboard functionality I think would require a new set of keypress event listeners.