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

The altKeyZoom attribute doesn't work as described #121

Open
happylilem opened this issue Nov 23, 2021 · 5 comments
Open

The altKeyZoom attribute doesn't work as described #121

happylilem opened this issue Nov 23, 2021 · 5 comments

Comments

@happylilem
Copy link

As stated in the documentation, when altKeyZoom is set to be true, zooming should only be enabled as the alt key is pressed. We've tried with multiple computers, and wouldn't be able to zoom or move the graph when the alt key is pressed. If I set altKeyZoom to false, the zooming and graph moving functions are enabled but they happen simultaneously with the brushing and filtering function, which causes inconvenience.

@happylilem
Copy link
Author

I just tried to replace the altKeyZoom attribute with modKeyZoom('Alt'), the latter one works! So I suspect the problem might arise from line 212 in diagram.js:
_diagram.modKeyZoom = _diagram.altKeyZoom = property(false);

@gordonwoodhull
Copy link
Contributor

Hey, sorry I haven’t had a chance to look at this.

I think it is probably a documentation bug. It is definitely delicate and I am sure there is a better system, but there is usually a way to get it working, so I’ve never cleaned it up.

In situations like this it’s probably best to look for an example that does work and then see what it did.

Let’s see if we can document this better, and deprecate one of the APIs if the name is misleading.

@happylilem
Copy link
Author

Thanks for the reply! And yes, I think the documentation could be further improved because the 'altKeyZoom' attribute could be found by entering into the search bar but not 'modKeyZoom'. However, I don't quite understand why make them two different attributes while they should serve the same function (zoom with Alt key pressed). ('altKeyZoom' is used in the brushing and filtering demo, and 'modKeyZoom' is used in the drag-drop composition demo.)
On the other hand, I still think there's bug within the 'altKeyZoom' attribute because after my attempts I found that when it's set to false, the graph can be zoomed and moved; when it's true, the graph cannot be zoomed or moved (even after I pressed Alt).

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Nov 30, 2021

IIRC what happened here is that we tried to add new functionality in a backward-compatible way, but we slipped up.

Originally it was a boolean altKeyZoom that allowed you to turn the feature on or off. Then we tried to generalize it to modKeyZoom taking a string or an array of strings, all of which must match the current modifier keys pressed, in order for the "mode" to be activated.

We probably tried to upgrade boolean into "Alt" at first, but broke it as we continued to change things.

So altKeyZoom should really be deprecated, or if it totally doesn't work, it should be outright removed.

The key-matching logic is here:

_mode.modKeysMatch = function(keys) {
if(!keys || keys === [])
return _pressed.empty();
if(!Array.isArray(keys))
keys = [keys];
var p = pressed();
if(p.length !== keys.length)
return false;
return keys.slice().sort().every(function(k, i) { return k === p[i]; });
};

Please let me know if using the string "Alt" works for you.

@happylilem
Copy link
Author

I just tried AltKeyZoom with 'Alt', and it worked! So it wasn't successfully set to Boolean and still carries the same function as modKeyZoom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants