Rangeslider allow zoom on oppaxis #2364

TomDemulierChevret · 2018-02-14T17:20:33Z

By default we can't currently zoom on the y-axis if we set a rangeslider on the x-axis.
If we write fixedrange: false in our y-axis config, it will override the lock put by the rangeslider and allow us to zoom both in x-and y-axis.

The issue is that the rangeslider is kinda broken if we zoom on y-axis since it will not keep the initial range on y-axis (like it does for the x-axis).

This PR :

maintain the correct y-axis range for data previsualization in the rangeslider even when the user zoom
add a visual clue to show the user where he has zoomed alongside the y-axis

Here is a set of screenshot to illustrate the issue and the PR :

Without zoom

With zoom on y-axis before PR

With zoom on y-axis after PR

If you have multiple y-axis and not all of them are zoomable, then you must add anchor: 'y' in your x-axis config (with y replaced with the name of one of the zoomable y-axis).

etpinard · 2018-02-15T19:06:13Z

Thanks for the PR. Impressive work and clear explanation!

But I have to say I'm not 100% sure the current behavior is incorrect. It might just be a matter of preference. Perhaps we can add an attribute e.g. xaxis.rangeslider.anchor: 'fixed' || 'mirror' (we can probably come up with a better name though 🤔 ) to toggle between the current behavior and yours.

TomDemulierChevret · 2018-02-16T10:20:13Z

Just added a new attributes perpendicularaxesinitialrange to switch between the new and old behaviour.
I put the default to new behaviour as the old one may not be the most intuitive.

TomDemulierChevret · 2018-02-16T15:05:58Z

Not really sure why the last test is failling, any hint from your side ?

etpinard · 2018-02-16T15:31:39Z

src/components/rangeslider/attributes.js

@@ -89,5 +89,17 @@ module.exports = {
            'If visible, perpendicular axes will be set to `fixedrange`'
        ].join(' ')
    },
+    perpendicularaxesinitialrange: {


This is probably a little to verbose for our needs. Attribute names rarely have more than two words smashed together.

The best option that comes to my mind is xaxis.rangeslider.fixedanchor: false || true, but anchor is used already elsewhere to declare a different thing.

Looking for opinions from @alexcjohnson @cldougl and @chriddyp

Yeah, I find it also too verbose but I prefered to use a clear name until we have a better proposal (since anchor is already used).

given that the range slider is always an x axis, we could use something like fixedyrange - and if we ever make a y axis range slider it can get a fixedxrange attribute.

It does makes sense, I was not quite sure rangeslider was for x-axis only.

etpinard · 2018-02-16T15:39:24Z

Not really sure why the last test is failling, any hint from your side ?

We've been fighting some intermittent test failures lately. Your latest commit is now ✅

alexcjohnson · 2018-02-16T16:46:16Z

referencing also #2172 - I love the effect here @TomDemulierChevret, just want to make sure we have a path forward when we add potentially stacked multiple axes. I guess it just means we'd end up with multiple clear and shaded regions stacked up, which should be fine.

TomDemulierChevret · 2018-02-16T17:24:36Z

By stacked multiple axes, you're referencing multiple x-axis right ?
Because if you're talking only about multiple y-axis, only one instance of each [clear/shaded] region is added.

edit : just saw the issue you're referencing.
I don't really know how it will behaved since I never experienced this use case (and doesn't know how exactly you will handle it).
But your proposal does makes sense.

alexcjohnson · 2018-02-16T21:22:51Z

src/plots/cartesian/axes.js

@@ -389,6 +389,10 @@ axes.saveRangeInitial = function(gd, overwrite) {
            ax._rangeInitial = ax.range.slice();
            hasOneAxisChanged = true;
        }
+        // store the initial range for the rangeslider if we zoom on oppaxis
+        if((isNew && ax.fixedrange === false) || (overwrite && hasChanged)) {
+            ax._rangesliderInitialRange = ax.range.slice();


Hmm, one issue with this: _rangesliderInitialRange isn't part of the figure JSON... so if you zoom in on the y axis, then you decide you like the plot and you save it, or you snapshot it even, the rangeslider y axis will not have the same range in the snapshot or when you reopen the plot.

That seems to me to mean that we need a real attribute for the range of each y axis on the rangeslider, and perhaps even an attribute for whether to autorange that y axis on the range slider - it would be cool if the y axis could be told to autorange in the range slider even if you start out zoomed in on the main plot, wouldn't it?

BTW I came to this realization as I was thinking "what tests should we have for this feature", noting that a big part of this is drawing code so it should have an image test, but then realizing as it stands we can't make an image test because we have no mechanism to force _rangesliderInitialRange to be different from the y axis range in an image test, therefore we can't make the opp axis masks appear there.

Right, I didn't think about that.
I'll look into it.

…rangeslider and save them

TomDemulierChevret · 2018-02-19T16:34:36Z

Added new attributes to the figure JSON as suggested by @alexcjohnson.
It allows to make the rangeslider y-axis work in 3 ways :

rangesliderFixedrange: false => the range is the same than the one displayed on the main plot (old behaviour before the PR)
rangesliderFixedrange: true => the range is fixed and set via rangesliderRange: [min, max]
rangesliderFixedrange: 'auto' [default] => the range is fixed and set via the range of the y-axis (or autorange if none provided)

Not sure exactly what id did broke for the plot_api React test to fail.

alexcjohnson · 2018-02-20T04:49:16Z

Good call putting the new attributes into the y axis - originally I was thinking that they belonged in the rangeslider container, but that wouldn't support multiple y axes correctly. A couple of tweaks I'd suggest:

We should only coerce these attributes if letter === y.
Instead of rangesliderFixedrange: (true | false | 'auto') I might suggest rangeslidermode: ('fixed' | 'match' | 'auto') - I think that's a bit more descriptive, and we've been moving away from these mixed-type attributes anyway. Also despite this being javascript, for portability we use only lowercase in attribute names (so rangesliderRange -> rangesliderrange). @etpinard opinions about this?
There may be places (Axes.expand?) where we need to check for rangeslidermode: 'auto' alongside autorange: true, in order to make sure the autorange precursor calculations are done. To test this: make a plot where the y axis is NOT autoranged at the beginning, but it IS supposed to be autoranged in the rangeslider, and see if it gets handled correctly.

I'll try out plot_api_test on this branch tomorrow, see if I can figure out what the problem is there.
the image test failed because we had master in an improper state that just got fixed late this afternoon - if you merge in master again, that one should work again.

…-zoom-on-oppaxis

etpinard · 2018-02-20T15:11:02Z

src/components/rangeslider/constants.js

    // style constants

    maskColor: 'rgba(0,0,0,0.4)',
+    maskOppColor: 'rgba(0,0,0,0.2)',


Just noticed this. Why pick a different opacity value here?

I like this effect - you see that the y axis is constrained, but it's not shaded as darkly as the x axis constraint, which aligns with the fact that you can only affect the x axis constraint with the range slider handles.

But a different thought occurred to me on looking at this: this is semitransparent black, so does that mean range sliders don't work on a black background? That's a totally separate issue, doesn't need to be addressed in this PR, but we should look into it.

Indeed, that's what I wanted to show with the 2 different values of opacity.

And yeah, the black backgrond will be an issue.

etpinard · 2018-02-20T15:40:40Z

Hmm. I think people will inevitably confuse yaxis?.rangesliderrange with xaxis?.rangeslider.range, especially when we implement y-axis (i.e. vertical) range sliders.

Perhaps, it would have been best to make range sliders a root layout component? 😡

Oh well, for now I can think of 3 options:

//  option 1
xaxis: {
  rangeslider: {
     // ...
   }
},
yaxis: {
   // finding a good concise name for this thing is very hard
   oppositerangeslider: {
      mode: '  ',
      range: [/* */],
   }
}

// option 2
xaxis: {
  rangeslider: {
     yrangemode: ' ',
     yrange: [/* */],
     y2rangemode: ' ',
     y2range: [/* */],
     // ...
  }
},

// option 3 - which would be more similar to other 
// attributes with axis counters
xaxis: {
   rangeslider: {
     rangemodey: ' ',
     rangey: [/* */],
     rangemodey2: ' ',
     rangey2: [/* */],
     // ....
}

After writing the 3 options down, I'd say I prefer option 3 by far. It keeps range sliders attributes contained in their base x-axis object. While axis counters (e.g. 'y', 'y2', 'y3', ...) aren't great, they are already a pretty big part of the plotly.js api. Thoughts?

alexcjohnson · 2018-02-20T16:05:19Z

After writing the 3 options down, I'd say I prefer option 3 by far.

I was hoping to avoid something like that, but I just realized there's a use case where we need to do it this way. Kinda convoluted, but imagine you have two side-by-side x axes sharing a y axis, each x axis has its own range slider, and you want different range settings for the y axes in the two rangesliders. Would be weird, as the y axes on the main plots are one axis so they must share a range... but I guess I can imagine wanting to restrict the range of the y axes in each range slider to just the data on the respective subplot.

This is not possible with yaxis.rangesliderrange - one way or another the attribute path must include both axis ids/names.

Option 3 is pretty good, but let me add one more to the mix:

xaxis: {
  rangeslider: {
    yaxis: {range: [/* */], rangemode: ' '},
    yaxis2: {range: [/* */], rangemode: ' '},
  }
}

alexcjohnson · 2018-02-20T16:09:06Z

(@TomDemulierChevret sorry for the back-and-forth here, attribute specs can be really tricky to get right, especially when we start thinking of all the possible edge cases involved)

etpinard · 2018-02-20T16:24:05Z

@alexcjohnson oh right, true. A given y axis can be linked to multiple x range sliders in general. Nice catch!

Option 3 is pretty good, but let me add one more to the mix:

I like it, but I still prefer my option 3 by a slight margin (mainly due to possible confusion between layout.yaxis and layout.xaxis.rangeslider.yaxis), but since we might have to add more opposite axis attributes down the road (e.g. grid line style in the range slider panel), your solution should scale better.

We could also do something like:

xaxis: {
  rangeslider: {
      oppaxis: {
          rangemode: ' ',
          range: [/* */]
       },
       oppaxis2: {

       }
   }
}

that way, the same oppaxis? containers could be used in future yaxis.rangeslider containers?

But really, maybe we should thinking about pulling rangeslider out of the axis containers entirely, wouldn't something like this be easier to manage and scale:

layout: {
  xaxis: {},
  yaxis: {},
  yaxis2: {},

  rangesliders: [{
    xref: 'x',
    yref: ['y', 'y2'],

    xaxis: {
      range: [/* */], // equivalent to current `xaxis.rangeslider.range`
      /// style attributes ...
    },
    yaxis: { 
      rangemode: ' ',
      range: [/* */]
    }
  }]

etpinard · 2018-03-07T16:42:43Z

@TomDemulierChevret would if be ok if we start pushing a few commits to this branch directly? We're very close to merging your PR. Thanks again for all your efforts!

TomDemulierChevret · 2018-03-07T16:44:37Z

Yes of course, no problem.

etpinard · 2018-03-07T17:07:25Z

test/jasmine/tests/validate_test.js

+            yaxis3: {}
+        });
+
+        expect(out).toBeUndefined();


As promised in https://github.com/plotly/plotly.js/pull/2364/files#r171957082

Please note, the _isSubplotObj flag in the range slider opposite axis attributes is very important. Without it, this test would spit out:

that is Plotly.validate would think that rangeslider.yaxis2 and rangeslider.yaxis3 aren't part of the schema.

Note that this appears to work in plotly.py aswell.

Copying the new plot-schema.json over there gives:

i.e. does not fail like:

cc @cldougl @Kully

…ulierChevret/plotly.js into rangeslider-allow-zoom-on-oppaxis

etpinard · 2018-03-07T18:53:33Z

src/plots/cartesian/axis_defaults.js

+    // the logic is complicated to figure this out later, particularly for y axes since
+    // the settings can be spread out in the x axes... so instead we'll collect them
+    // during supplyDefaults
+    containerOut._rangesliderAutorange = false;


Very clear comment. Thanks!

etpinard · 2018-03-07T18:57:25Z

src/components/rangeslider/defaults.js

                rangemodeDflt = 'fixed';
            }

            var rangeMode = coerceRange(rangeContainerIn, rangeContainerOut, 'rangemode', rangemodeDflt);
            if(rangeMode !== 'match') {
-                coerceRange(rangeContainerIn, rangeContainerOut, 'range', layoutOut[yName].range.slice());
+                coerceRange(rangeContainerIn, rangeContainerOut, 'range', yAxOut.range.slice());


Ok. Looks like rangeslider.yaxis?.range is still getting coerced when rangemode: 'auto' though this appears standard for subplot xaxis and yaxis range under autorange: true, so this sounds fine 👌 .

That said, it would be nice in the future to make:

Plotly.validate([], { xaxis: { autorange: true, range: [0, 1] } })

spit out an unused log message for xaxis.range

etpinard · 2018-03-07T18:57:44Z

test/image/mocks/range_slider_rangemode.json

+      },
+      "title": "Rangeslider Y2 rangemode fixed"
+    },
+    "yaxis": {"type": "log", "range": [2, 6], "title": "Y explicit range"},


etpinard · 2018-03-07T19:09:37Z

src/components/rangeslider/oppaxis_attributes.js

+        values: ['auto', 'fixed', 'match'],
+        dflt: 'match',
+        role: 'style',
+        editType: 'calc',


@alexcjohnson we should be able to change these to editType: 'plot', correct?

oh hmm... actually I don't think we can, it's like ax.autorange which is still calc, because this affects _rangesliderAutorange which determines whether we even bother calculating ax._min/_max during the calc step.

Ok. I tried switching them to 'plot', but that didn't break any test 😕

etpinard · 2018-03-07T19:14:48Z

test/jasmine/tests/range_slider_test.js

+        .then(done);
+    });
+
+    it('should update rangeslider x/y ranges when data changes even if main axes are not autoranged', function(done) {


Thanks for testing this case!

etpinard · 2018-03-07T19:34:06Z

All right. Time to merge to thing 💃

I'll let @alexcjohnson do the honors after a failing test case for #2364 (comment) is added.

1.35.0 will be out in the few hours.

Thanks a million to @TomDemulierChevret for first bringing this issue and persevering through our numerous iterations at solving this problem 💪

I'd like to make this PR an early nominee for community PR of the year 🏆

this test fails if rangeslider.yaxis.rangemode or rangeslider.(auto)range doesn't have editType: 'calc'

xapple · 2018-07-29T09:55:38Z

So how do we make the range slider keep a constant y axis with the latest version of plotly? What's the configuration?

better visual cue of zoom on oppaxis with rangeslider

519e1a7

add a new attributes to rangeslider to switch back to the old behaviour

044e633

TomDemulierChevret added 2 commits February 16, 2018 11:59

fix check when using new data

f335258

fix default value rangeslider test

707f432

etpinard added feature something new status: in progress labels Feb 16, 2018

etpinard reviewed Feb 16, 2018

View reviewed changes

renammed new attributes to "fixedyrange"

6a7f652

alexcjohnson reviewed Feb 16, 2018

View reviewed changes

TomDemulierChevret added 4 commits February 19, 2018 16:18

Introduce new attributes to axes to allow to specifiy fixed range to …

fc4f082

…rangeslider and save them

Fix broken test from attribute removal

92c6550

Add missing gl attribute

b30167c

Merge branch 'master' into rangeslider-allow-zoom-on-oppaxis

91caff1

TomDemulierChevret added 4 commits February 20, 2018 13:39

Renammed new attributes for more clarity and portability

3dd91b5

Coerce of new attributes is only made for y axis

00fd1d7

Merge remote-tracking branch 'upstream/master' into rangeslider-allow…

5e0486a

…-zoom-on-oppaxis

Changed rangeslidermode values for more clarity

32488f1

etpinard reviewed Feb 20, 2018

View reviewed changes

TomDemulierChevret added 4 commits March 7, 2018 11:26

Add test for new attributes default and coerce

9c66f81

Add interaction test for new attributes

f092c44

Fixed autorange test precision

041ce3d

Fixed autorange test precision

23c8401

etpinard mentioned this pull request Mar 7, 2018

Fix rangeslider titles with stacked y axes #2451

Merged

Merge branch 'master' into rangeslider-allow-zoom-on-oppaxis

73560f6

alexcjohnson mentioned this pull request Mar 7, 2018

scatter trace fails to disappear on legend hide #2452

Closed

etpinard added 2 commits March 7, 2018 12:04

rename range_attributes file -> oppaxis_attributes

c556f8c

add rangeslider.yaxis? to schema flagging it with _isSubplotObj

43f15a7

etpinard reviewed Mar 7, 2018

View reviewed changes

etpinard added this to the v1.35.0 milestone Mar 7, 2018

alexcjohnson added 2 commits March 7, 2018 13:20

fix rangeslider.y.rangemode=auto and add a test image

ccb6091

Merge branch 'rangeslider-allow-zoom-on-oppaxis' of github.com:TomDem…

6a88b24

…ulierChevret/plotly.js into rangeslider-allow-zoom-on-oppaxis

etpinard reviewed Mar 7, 2018

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Mar 7, 2018

etpinard approved these changes Mar 7, 2018

View reviewed changes

alexcjohnson added 2 commits March 7, 2018 14:46

test turning on autorange via rangeslider attrs

95b8c10

this test fails if rangeslider.yaxis.rangemode or rangeslider.(auto)range doesn't have editType: 'calc'

drop rangeslider style attributes to editType: plot and test

4f54f62

alexcjohnson merged commit beaec8d into plotly:master Mar 7, 2018

TomDemulierChevret deleted the rangeslider-allow-zoom-on-oppaxis branch March 7, 2018 21:59

etpinard mentioned this pull request Jul 3, 2018

Templates #2761

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rangeslider allow zoom on oppaxis #2364

Rangeslider allow zoom on oppaxis #2364

TomDemulierChevret commented Feb 14, 2018 •

edited

Loading

etpinard commented Feb 15, 2018 •

edited

Loading

TomDemulierChevret commented Feb 16, 2018

TomDemulierChevret commented Feb 16, 2018

etpinard Feb 16, 2018 •

edited

Loading

TomDemulierChevret Feb 16, 2018

alexcjohnson Feb 16, 2018

TomDemulierChevret Feb 16, 2018

etpinard commented Feb 16, 2018

alexcjohnson commented Feb 16, 2018

TomDemulierChevret commented Feb 16, 2018 •

edited

Loading

alexcjohnson Feb 16, 2018

TomDemulierChevret Feb 19, 2018

TomDemulierChevret commented Feb 19, 2018 •

edited

Loading

alexcjohnson commented Feb 20, 2018

etpinard Feb 20, 2018

alexcjohnson Feb 20, 2018

TomDemulierChevret Feb 20, 2018

etpinard commented Feb 20, 2018

alexcjohnson commented Feb 20, 2018

alexcjohnson commented Feb 20, 2018

etpinard commented Feb 20, 2018

etpinard commented Mar 7, 2018

TomDemulierChevret commented Mar 7, 2018

etpinard Mar 7, 2018

etpinard Mar 7, 2018 •

edited

Loading

etpinard Mar 7, 2018

etpinard Mar 7, 2018

etpinard Mar 7, 2018

etpinard Mar 7, 2018

alexcjohnson Mar 7, 2018

etpinard Mar 7, 2018

etpinard Mar 7, 2018

etpinard commented Mar 7, 2018

xapple commented Jul 29, 2018

Rangeslider allow zoom on oppaxis #2364

Rangeslider allow zoom on oppaxis #2364

Conversation

TomDemulierChevret commented Feb 14, 2018 • edited Loading

Without zoom

With zoom on y-axis before PR

With zoom on y-axis after PR

etpinard commented Feb 15, 2018 • edited Loading

TomDemulierChevret commented Feb 16, 2018

TomDemulierChevret commented Feb 16, 2018

etpinard Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Feb 16, 2018

alexcjohnson commented Feb 16, 2018

TomDemulierChevret commented Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomDemulierChevret commented Feb 19, 2018 • edited Loading

alexcjohnson commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Feb 20, 2018

alexcjohnson commented Feb 20, 2018

alexcjohnson commented Feb 20, 2018

etpinard commented Feb 20, 2018

etpinard commented Mar 7, 2018

TomDemulierChevret commented Mar 7, 2018

Choose a reason for hiding this comment

etpinard Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Mar 7, 2018

xapple commented Jul 29, 2018

TomDemulierChevret commented Feb 14, 2018 •

edited

Loading

etpinard commented Feb 15, 2018 •

edited

Loading

etpinard Feb 16, 2018 •

edited

Loading

TomDemulierChevret commented Feb 16, 2018 •

edited

Loading

TomDemulierChevret commented Feb 19, 2018 •

edited

Loading

etpinard Mar 7, 2018 •

edited

Loading