Make scrollZoom config option a flaglist by etpinard · Pull Request #3422 · plotly/plotly.js

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard · 2019-01-08T23:04:23Z

An attempt at resolving #143 and #2998 as well as a similar issue-less problem for mapbox subplots.

In brief, this PR makes the config option scrollZoom a flaglist -- with flags for all subplots that support zooming via scroll -- instead of a boolean:

its default value now becomes gl3d+geo+mapbox (instead of false) as all three of these subplot types have scroll zoom enabled by default.
Setting scrollZoom:true enables scroll zoom for all subplots, which is equivalent to scrollZoom:'cartesian+gl3d+geo+mapbox'
Setting scrollZoom:false disables scroll zoom on all subplots, which should make a bunch of users 😄

@plotly/plotly_js what do you think?

- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

etpinard · 2019-01-08T23:06:18Z

... to be merge into #3376 where I made use of the new plot_config option declarations.

alexcjohnson · 2019-01-08T23:32:49Z

Nice solution, I like it!

etpinard · 2019-01-14T14:42:37Z

@archmoj would you mind taking a look at this one at some point this week?

archmoj · 2019-01-14T15:03:22Z

@etpinard OK. That's interesting. I'll have a look.

archmoj

Great arrangement. Thanks @etpinard.
Please find my comments below.

archmoj · 2019-01-16T16:26:37Z

Thanks for the revision!
💃

etpinard · 2019-01-16T16:31:27Z

No problem! Thank you for the review!

zfei · 2023-01-31T15:03:20Z

+ scene.glplot.canvas.addEventListener('wheel', function() {
+ if(gd._context._scrollZoom.gl3d) {
+ relayoutCallback(scene);
+ }
+ }, passiveSupported ? {passive: false} : false);


I've disabled scrollZoom in the plot config, but the mouse is still getting captured when scrolling by the canvas. Looks like it's because of the wheel event listener that's being registered here anyway. After I remove the wheel listener on scene element from chrome inspector, scrolling the page no longer gets captured by the canvas.
@etpinard, can we not add the listener at all if scrollzoom is disabled?

@zfei thanks for the debugging. This comment is going to get lost here on a merged PR though, can you make a new issue for this? Or better yet a PR, looks like it's probably a pretty easy fix 😎

Thanks @alexcjohnson.
I've never touched the Plotly codebase and haven't set up my mind to go through the learning curve yet. 😉
Will just leave it as an issue for now.

etpinard added 4 commits January 8, 2019 17:53

make scrollZoom a flaglist w/ dflt 'gl3d+geo+mapbox'
c83538a
- which match the current "default", but will make scrollZoom:false disable zoom for all subplots!

resolves#143- implement scrollZoom:false for geo subplots
b56591d

resolves#2998- implement scrollZoom:false for gl3d subplots
26618dd

implement scrollZoom:false for mapbox subplots
16a01e7

etpinard added feature something new status: reviewable labels Jan 8, 2019

etpinard added this to the 1.44.0 milestone Jan 8, 2019

archmoj self-requested a review January 14, 2019 15:03

archmoj reviewed Jan 16, 2019
View reviewed changes

Comment threadsrc/plots/gl3d/camera.js
Comment threadtest/jasmine/tests/config_test.js
Comment threadtest/jasmine/tests/config_test.js
Comment threadsrc/plot_api/plot_api.js

explicitly assert that _scrollZoom.cartesian isn't set
09f1af5

archmoj mentioned this pull request Jan 16, 2019
Add config options in plot schema #3376
Merged

etpinard merged commit ceac510 into config-opts-in-plot-schemaJan 16, 2019

etpinard deleted the scrollzoom-flaglist branch January 16, 2019 16:31

alexcjohnson mentioned this pull request Jan 16, 2019
Plot & page scrolling #3252
Closed
5 tasks

This was referenced Jan 16, 2019
No way to disable mouse-scroll zoom for geo subplots #143
Closed
scatter3d zooms on scroll even if zoomScroll is false #2998
Closed

This was referenced Apr 5, 2019
Mapbox doesn't immediately respond to scrollZoom: false #3738
Closed
Graph config defaults don't match plotly.js plotly/dash-core-components#514
Closed

zfei reviewed Jan 31, 2023
View reviewed changes