Uh oh!
There was an error while loading. Please reload this page.
Do not disable scrollZoom scroll{Width,Height} > client{Width,Height} - #3424
Conversation
Used to disable scrollzoom when the plot had scrollbars. Ended up with a plot which is not zoomable with mouse scroll on a page which had additional content (had a scrollbar).
etpinard
commented
Jan 14, 2019
@antoinerg would you mind taking a look at this one at some point this week? |
etpinard
commented
Jan 14, 2019
Oh wait. I still have to add a few tests for this one. Asking for @alexcjohnson 's opinion on this potentially-controversial patch. |
alexcjohnson
commented
Jan 14, 2019
Agreed. Explicit is better than implicit. |
@antoinerg I know you're busy, but would you mind taking a 👁️ at this PR at some point this week (should be quick 😉 ) |
| if(pc.scrollHeight - pc.clientHeight > 10 || | ||
| pc.scrollWidth - pc.clientWidth > 10) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I am not sure what this early return was meant to achieve but I guess it's OK to 🔪 considering it doesn't have any associated tests...
antoinerg
commented
Jan 15, 2019
Thank you for the fix @etpinard! It seems to work well: https://codepen.io/anon/pen/roRdmB If the removed code really is a relic, then 💃 |
fixes#2371 and #3337 by including a commit from @marfoldi's #3085.
Removing the
scroll{Width,Height} > client{Width,Height}early return fixes two bugs:{responsive: true, scrollZoom: true}+ graph div heightvhunits doesn't work, as in that scenariogd.client{Width,Height}is0leading to an early return.There are probably other ways to fix these bugs, but that
scroll{Width,Height} > client{Width,Height}early return feels out-of-place. It's probably a relic from the workspace days, where we setscrollZoom:true, but wanted some extra do-not-scroll logic. As scroll zoom is disable by default for cartesian subplots, I don't think overriding ascrollZoom: trueuser config is a good idea.I hope @plotly/plotly_js agrees. If so, I'll add a few tests.