Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to enable both wheel and drag modes at the same time #507

Merged
merged 9 commits into from
May 14, 2021

Conversation

jledentu
Copy link
Collaborator

Hi,

Here is a PR thats adds the ability to enable both wheel and drag modes at the same time, as suggested in #239. I propose to split the wheel/drag configurations like this:

zoom: {
      wheel: {
        enabled: true,
        speed: 0.1,
        modifierKey: null
      },
      drag: {
        enabled: true,
        backgroundColor: 'yellow',
        borderColor: 'black',
        borderWidth: 1,
        threshold: 1
      },
      mode: 'xy',
}

speed, threshold, wheelModifierKey (now wheel.modifierKey) are moved to their respective configurations, I think that makes clearer that a given option refers to a given mode.

@kurkle
Copy link
Member

kurkle commented May 12, 2021

Nice!
How about pinch zoom? I think its still checking for zoom.enabled. Maybe it should be possible to disable that separately? Own scope for it?

@jledentu
Copy link
Collaborator Author

Oh you're right, I admit that I often forget to check touch behaviors. Do I add pinch.enabled? I guess that it will often be aligned with wheel.enabled as I don't know if it makes sense to explicitly enable zoom by mouse and disable zoom on touch devices. But at least we allow more flexibility.

@kurkle
Copy link
Member

kurkle commented May 12, 2021

I think pinch.enabled would be good. Maybe fall back from each scope to zoom.enabled, to make the "break" of the change smaller and maybe easier to find.

Edit: Other option would be to add a warning when zoom.enabled is set, about it having no effect. Maybe in the start hook, so it won't be spammy.

kurkle
kurkle previously approved these changes May 12, 2021
@kurkle kurkle requested a review from etimberg May 12, 2021 11:30
@jledentu
Copy link
Collaborator Author

@kurkle I added pinch.enabled + a warning on zoom.enabled as suggested (I think it's better to remove this option in order to keep the code clean and make the configuration more explicit?).

@kurkle kurkle linked an issue May 12, 2021 that may be closed by this pull request
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two very minor comments. Looks great!

docs/guide/options.md Show resolved Hide resolved
src/plugin.js Outdated Show resolved Hide resolved
@kurkle kurkle added this to the 1.0.0 milestone May 14, 2021
@kurkle kurkle merged commit 5ed4e2a into chartjs:master May 14, 2021
@jledentu jledentu deleted the both-wheel-and-drag-zoom branch May 17, 2021 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When drag zoom is enabled, scroll zooming is blocked
3 participants