Add keybindings for zoom in, zoom out and zoom reset#1629
Add keybindings for zoom in, zoom out and zoom reset#1629glennsl merged 2 commits intoonivim:masterfrom artsem-teal:feature/workspace/zoom-keybindings
Conversation
|
|
glennsl
left a comment
glennsl
left a comment
There was a problem hiding this comment.
Awesome @ArtemDrozdov. I haven't tried it out yet, just looked over the code, but it seems sound to me! I have a few suggestions and a reorganization request below though.
src/Store/VimStoreConnector.re
Outdated
| let zoomStep = 0.2; | ||
| let defaultZoom = 1.0; | ||
| let minZoomValue = 0.0; | ||
| let maxZoomValue = 2.8; |
There was a problem hiding this comment.
Could you put these in a Constants sub-module at the top of the file?
There was a problem hiding this comment.
Yes, of course.
src/Store/VimStoreConnector.re
Outdated
| let calculatedZoomValue = | ||
| switch (zoomAction) { | ||
| | ZoomIn => currentZoomValue +. zoomStep | ||
| | ZoomOut => currentZoomValue -. zoomStep | ||
| | ZoomReset => defaultZoom | ||
| }; |
There was a problem hiding this comment.
What's the reason you pass in a zoomAction instead of just passing in the value directly?
There was a problem hiding this comment.
What do you mean by
instead of just passing in the value directly
Can't get your point.
There was a problem hiding this comment.
Looking closer, I guess you can't pass in currentZoomValue +. zoomStep) directly instead of ZoomIn because you need currentZoomValue. But you could do zoomEffect(state, zoom => zoom +. zoomStep).
src/Store/VimStoreConnector.re
Outdated
| let newZoomValue = | ||
| calculatedZoomValue < minZoomValue | ||
| ? minZoomValue | ||
| : calculatedZoomValue > maxZoomValue | ||
| ? maxZoomValue : calculatedZoomValue; |
There was a problem hiding this comment.
You can use Utility.IntEx.clamp for this. It would be significantly more readable I think.
src/Store/VimStoreConnector.re
Outdated
| | Command("workbench.action.zoomIn") => ( | ||
| state, | ||
| zoomEffect(state, ZoomIn), | ||
| ) | ||
| | Command("workbench.action.zoomOut") => ( | ||
| state, | ||
| zoomEffect(state, ZoomOut), | ||
| ) | ||
| | Command("workbench.action.zoomReset") => ( | ||
| state, | ||
| zoomEffect(state, ZoomReset), | ||
| ) |
There was a problem hiding this comment.
I don't see any reason for these and the effect to need to live in VimStoreConnector. Could you move it to CommandStoreConnector instead (unless there's a reason I don't see)?
|
Pushed changes according @glennsl comments:
|
glennsl
left a comment
glennsl
left a comment
There was a problem hiding this comment.
Looks and works great. Thanks @ArtemDrozdov!
|
Oh, CI fails on an unused variable warning. That should be a quick fix though. |
3 participants
Added keybindings for zoom in, zoom out and zoom reset according #1423 issue.
I checked zooming behavior in VSCode and they have next logic: default value is 0, minimum is -8 and maximum is 9. Zooming step is 1 which equals to 20%.
Short summary what was done in this PR:
Minimum value for zooming is 0 because on lower values there is no changes in UI.
And probably there should be some logic to process Cmd + = on Mac and Ctrl + = on Linux/Windows for example. Because with current implementation both of those combinations are working on Mac.
Would love to hear any comments and suggestion!