Keyboard zoom#293
Conversation
| </div> | ||
| </div> | ||
| <div> | ||
| <p id="zoomTooltip">Alt +/- to zoom</p> |
There was a problem hiding this comment.
That would need to be localized, otherwise, it's static in all languages.
But let's first see whether that is a the appropriate approach.
For more information, please see the MDN guide on how to localize WebExtensions and our internationalization (i18n) guide.
| function zoomQrCode(e) { | ||
| var evtobj = window.event ? event : e; | ||
| if (evtobj.keyCode == 61 && evtobj.altKey && qrLastSize < 440) { | ||
| setNewQrCodeSize(qrLastSize + 30, true); |
There was a problem hiding this comment.
Note setNewQrCodeSize may – depending on
So yeah, I dunno whether my idea of remembering it additionally, was a good idea, but it seems okay'ish.
| } | ||
|
|
||
| // zoom listener | ||
| document.onkeydown = zoomQrCode; |
There was a problem hiding this comment.
IMHO better use addEventListener it's the modern and better syntax.
|
|
||
| localStorage.setItem("lastSize", qrLastSize); | ||
| } | ||
| if (evtobj.keyCode == 173 && evtobj.altKey && qrLastSize > 50) { |
There was a problem hiding this comment.
keyCode is kinda deprecated, see the warning at https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode, so let's better switch to alternatives, if possible.
rugk
left a comment
rugk
left a comment
There was a problem hiding this comment.
Hi @eridilla,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃
I am a little critical with having such a non-standard tooltip like Alt+ (+/-) for zoomimng, given that as I said in #189 (comment) mouse wheel and zoom also already works.
It's a good idea to give a hint, but maybe that is a little too obtrusive (at best it should work intuitively, but I see why that was not possible, as you've explained in #189 (comment)).
So the approach I would recommend, as that is not the most important feature here, would be to add a tip via the (already integrated) RandomTips library.
Also I need to test this out, of course.
A general note, please: Next time avoid reformatting many unrelated files, that are not related to the change. The format looks okay, so I will keep that (dunno how that happened? EsLint?), but it makes reviewing harder. Or, at least, do it in a separate commit.
Also, BTW: Next time, try to avoid doing a pull request from the master branch, because you can run into problems when you have a "non-clean" master that does not follow this repo here (i.e. "upstream"). See this article for details. Anyway, this is only a tip for the next time. 😃
|
Hey @rugk, I looked at your comments and fixed accordingly. I rewrote the key press listener to the addEventListener function and replaced the deprecated keyCodes. I removed the div with the tooltip and added the tip to Tips.js and the english messages.json. When I was trying out the random tip I got it to show up incorrectly a few times, then changed my code to look like the other tips and then I couldn't get any tip to show at all so I hope the additions I made are correct. Also sorry about the reformatting, Prettier did that on it's own, next time I'll check harder to make sure that doesn't happen again. 😅 Thank you once again for your comments and I hope these changes are better. 😀 |
| function zoomQrCode(e) { | ||
| var evtobj = window.event ? event : e; | ||
| if (evtobj.keyCode == 61 && evtobj.altKey && qrLastSize < 440) { | ||
| document.addEventListener('keydown', function(event) { |
There was a problem hiding this comment.
naaaw, I would still keep the function zoomQrCode or so you had… You can just reference that here AFAIK.
| localStorage.setItem("lastSize", qrLastSize); | ||
| } | ||
| if (event.key == '-' && event.altKey && qrLastSize > 50) { | ||
| setNewQrCodeSize(qrLastSize - 30, true); |
There was a problem hiding this comment.
Ah and could you extract the 30 into a const KEYBOARD_ZOOM_SIZE = 30 constant or so? Magic numbers are not a good practise in source code that should be readable… 🙃
There was a problem hiding this comment.
Same for 50 which I assume is something as MINIMAL_ZOOM_SIZE or so?
Just place the constants at the top, there should be some, already. 😉
2 participants
Issue #189