Add a modifierKey for drag-to-zoom #556
Conversation
|
There was a problem hiding this comment.
I left a couple of comments.
The other pr is about half the size, but we are still talking about couple of bytes. I ofc prefer the way I did it :)
However, its easier to get this merged as I don't need to request a review from another maintainer.
I also noticed that pan.enabled can be false and a modifierKey in there would still prevent drag-to-zoom. I changed that in the other pr.
| for (const key of ['ctrl', 'alt', 'shift', 'meta']) { | ||
| for (const pressed of [true, false]) { | ||
| let chart, scaleX, scaleY; | ||
| it(`should ${pressed ? 'not ' : ''}change ${pressed ? 'without' : 'with'} key ${key}`, async function() { |
There was a problem hiding this comment.
The 'not'/'' and 'without'/'with' are reversed
There was a problem hiding this comment.
Whoops, good catch, thanks!
| const modifierKey = panOptions.modifierKey; | ||
| const requireModifier = modifierKey && (event.pointerType === 'mouse'); | ||
| if (!state.panning && requireModifier && !event.srcEvent[modifierKey + 'Key']) { | ||
| if (!state.panning && blockedByPanModifierKey(panOptions, event) || blockedByDragModifierKey(zoomOptions, event)) { |
There was a problem hiding this comment.
I think the !state.panning check is needed for both conditions; once panning is started, it should not change to drag-zoom if that modifier key is pressed while still panning.
|
Thanks for the feedback @kurkle! I would just go with your PR. It's the cleaner of the two, it's smaller and at least for me there's no immediate rush to get it merged. |
This PR adds the
modifierKeyoption tozoom.drag, in a similar fashion to how it works for panning and wheel zooming.Note: by some hilarious coincidence it looks like @kurkle had the exact same idea today and beat me by 4 hours to add this 😂 I noticed that I had a couple of changes not included in his PR, so I've decided to add my MR as well. I'm perfectly fine with either PR getting merged: I care more about the feature being there than getting my code merged 😉.