Fix staticPlot behaviour for range slider and legend

[miqh]

Hello,

I dug into #5177 and eventually arrived at some changes which I believe should fix the issues described.

I used the existing range_slider test mock to check the effect of the changes, which I've attached previews of below.

I suspect I also need to update some test fixture screenshots (i.e. baseline task?) because these changes effect visuals, but I'm not sure how to go about doing this.

[archmoj]

@miqh Thanks very much for the PR.
Regarding regenerating the baselines, please visit https://github.com/plotly/plotly.js/blob/master/CONTRIBUTING.md#image-pixel-comparison-tests.
At the moment I am not 100% sure if we should vote for dropping the drag handlers whereas they illustrate the endpoints on the full range.
We will discuss this tomorrow during our internal meeting.

[nicolaskruchten]

The handles only make sense for interaction I think... so long as the shading is still present I'm ok with dropping the handles in static mode

[archmoj]

The handles only make sense for interaction I think... so long as the shading is still present I'm ok with dropping the handles in static mode

Great!
We need to regenerate a number of baselines here.
@miqh after setting up with https://github.com/plotly/plotly.js/blob/master/CONTRIBUTING.md#image-pixel-comparison-tests
could you please trying the following command to make the new baselines?

npm run baseline axes_breaks-candlestick2 range_slider range_slider_initial_expanded range_slider_rangemode axes_breaks-finance axes_breaks-rangeslider candlestick_double-y-axis finance_multicategory ohlc_first range_slider_axes_double range_slider_initial_valid range_slider_reversed-range candlestick_rangeslider_thai range_slider_axes_stacked range_slider_legend_left range_slider_top_axis automargin-rangeslider-and-sidepush axes_breaks-candlestick axes_breaks-ohlc_candlestick_box finance_subplots_categories range_slider_box range_slider_multiple

[archmoj]

@miqh and if you had difficulty regenerating all these baselines, please let me know.
If you check the "allow upstream administrators to edit this PR" we should have push access to your branch.
See https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests
Or alternatively I could make a PR to your branch for that.
Thanks.

[archmoj]

The handles only make sense for interaction I think... so long as the shading is still present I'm ok with dropping the handles in static mode

@nicolaskruchten @alexcjohnson
After generating potential new baselines in d65674f,
I still think that displaying rangeslider handels is important.
Firstly, the graph does not look great if one or both endpoints are the start/end of the range.

Secondly, we do display similar selection handle for parcoords so I think we should keep them on the graph for rangeslider.

[nicolaskruchten]

Hmmm OK, let's leave them in as static elements I guess.

[archmoj]

Hmmm OK, let's leave them in as static elements I guess.

@miqh would you mind revising this PR in a way that we don't need to change any baseline?

[miqh]

Hello @archmoj,

Apologies for the tardy follow-up.

In any case, thanks for pointing out the instructions for running the image tests. Not quite sure how I missed them to begin with.

As per your request, I've made another commit which backs out of hiding the range slider grabbers when staticPlot is enabled.

However, I can't see how their visibilities would affect how plot margins are established. I had a look at d65674f, where you've presumably generated the baselines from my initial set of changes. I also can't locate the example you attached with the undesirable margin issue. If the presence of the grabbers does indeed affect the margin choice somewhere else in the codebase, then I guess this makes sense.

The closest diff that looks like that attached screenshot is probably this one:

[archmoj]

Thanks @miqh.
The changes now are minimal and looking great!
The final step is to add two jasmine tests for each component,
possibly here: https://github.com/miqh/plotly.js/blob/4c1cacb9e2d3d3f3cdd3af787b22e5a53ec78924/test/jasmine/tests/legend_test.js#L934
and here: https://github.com/miqh/plotly.js/blob/4c1cacb9e2d3d3f3cdd3af787b22e5a53ec78924/test/jasmine/tests/range_slider_test.js#L41
to ensure the interactions would be disabled with a staticPlot.

To run those two test suites you ould use the following command:

npm run test-jasmine -- legend range_slider 

[archmoj]

Nicely done.
💃

[josephernest]

Hi, do you know if this is a bug or not: even if the plot is in staticPlot: true, the slider still works (it should not):

https://jsfiddle.net/1Lymg5jq/

<meta charset="utf8">
<script src="https://cdn.plot.ly/plotly-latest.js"></script>
<div id="myDiv"></div>
<script>
var steps = [];
for (var i = 0; i < 100; i++) 
    steps.push({label: i, method: "skip"});
Plotly.newPlot("myDiv", [{ "y": [1, 2, 3] }], {sliders: [{ len: 1, x: 0, steps: steps }]}, {staticPlot: true});
</script>

[archmoj]

@josephernest Please note plotly-latest stayed at latest v1.
To load the latest current v2 version try https://cdn.plot.ly/plotly-2.16.4.min.js instead.

[josephernest]

Thanks @archmoj but the problem is still there with 2.16.4:

https://jsfiddle.net/2mgzy38j/

The plot is static, but the slider is still active (it should not).