Range slider second iteration (part 1) #962

etpinard · 2016-09-21T22:42:23Z

fixes #686

In this first instalment of the range slider second iteration push:

the draw routine has been rewritten using d3 enter/exit/update
- so creation / deletion / update are governed by the same code paths!
- this make the setRangeSliderRange relayout hook obsolete. The range slider simply updates according to the current xaxis.range and xaxis.rangeslider.range
the mousedown handler now creates a cover slip to avoid selecting text (e.g. axis labels) while dragging
range sliders now span the correct axis width (using xaxis.domain)
range sliders stay coherent after relayout calls!
all features coming from Feature range slider #336 / Rangeslider data conversion #441 and /Initial rangeslider ranges #473 are intact (I think)

- so that computed rangeslider range can be copied into input container, ensuring that the autorange routine isn't called on updates.

- use d3 enter/exit/update pattern to handle drawing step for creation / deletion and update paths -> no longer need a relayout hook !!! - fix: range slider span depends on axis range - fix: range slider option now update on relayout - use coverslip to avoid select axis title

- with proper class name and full 'translate' values

etpinard · 2016-09-21T22:43:44Z

src/components/rangeslider/draw.js

+            minVal = opts.d2p(axisOpts.range[0]),
+            maxVal = opts.d2p(axisOpts.range[1]);
+
+        var dragCover = dragElement.coverSlip();


Note: I didn't use the full dragElement routine because it slowed down the requestAnimationFrame calls considerably.

etpinard · 2016-09-21T22:45:08Z

src/components/rangeslider/draw.js

+
+    var rangeSliders = fullLayout._infolayer
+        .selectAll('g.' + constants.containerClassName)
+        .data(rangeSliderData, keyFunction);


As before, there can only be one range slider on a graph at once.

But, the drawing code is now general enough that adding multiple range sliders as a feature would be pretty easy.

etpinard · 2016-09-21T22:46:20Z

src/components/rangeslider/draw.js

+}
+
+function drawRangePlot(rangeSlider, gd, axisOpts, opts) {
+    var rangePlots = rangePlot(gd, opts._width, opts._height);


@mdtusz 's rangePlot wrapper is (still) is use here.

After #946, I hope to make obsolete.

Commits in https://github.com/plotly/plotly.js/compare/range-slider-range-plot indeed make the current rangePlot() obsolete. Those commits deserve a PR of their own - coming after this PR is merged.

mdtusz

I guess there's still not quite enough tolerance in those tests still for CI :/

Looks great though! Will hopefully make refactoring more to allow for vertical and multiple sliders much easier.

💃 once addressing the one comment, and tests ✅

mdtusz · 2016-09-22T13:51:31Z

src/components/rangeslider/defaults.js

@@ -48,4 +52,7 @@ module.exports = function handleDefaults(layoutIn, layoutOut, axName, counterAxe
            layoutOut[ax] = opposing;
        });
    }
+
+    // to map back range slider (auto) range
+    containerOut._input = containerIn;


Do we need to delete containerIn._input before this to avoid a loop?

what kind of loop are you referring to here?

containerOut._input = { /* the containerIn */, _input: { /* the previous containerIn */, _input: { /* the previous conta...

_input is always linked to the layout as passed to Plotly.plot(gd, data, layout) - which doesn't know anything about containerOut.

mdtusz · 2016-09-22T14:07:00Z

src/components/rangeslider/draw.js

+            .call(drawRangePlot, gd, axisOpts, opts)
+            .call(drawMasks, gd, axisOpts, opts)
+            .call(drawSlideBox, gd, axisOpts, opts)
+            .call(drawGrabbers, gd, axisOpts, opts);


My favourite method name.

rreusser · 2016-09-22T14:35:02Z

src/components/rangeslider/draw.js

+    var dataMin = clamp(opts.p2d(opts._pixelMin)),
+        dataMax = clamp(opts.p2d(opts._pixelMax));
+
+    window.requestAnimationFrame(function() {


I'm sure it's valid, but I'm curious what effect requestAnimationFrame has here. I'm not sure I've ever really used it without calling is pseudo-recursively. Is the goal to get it bumped down in priority below user interactions so that relayout doesn't block the UI?

Yep. It has a similar effect to setTimeout(something, 0)

Cool. I hadn't noticed it affecting interactions that way, but I believe it. I'm usually after the pause-while-backgrounded nature of the calls.

- sometimes the rangeslider range is cleared when no enter selection present, e.g. on relayout(gd, xaxis.rangeslider: { thickness: 0.2 })

etpinard · 2016-10-06T02:54:54Z

test/jasmine/assets/custom_matchers.js

+
+    toBeWithin: function() {
+        return {
+            compare: function(actual, expected, tolerance, msgExtra) {


ping @rreusser @monfera for future test cases

Nice. I found toBeCloseTo a bit unpleasant in the way it uses digits of precision instead of just a tolerance.

mdtusz · 2016-10-06T15:30:26Z

Lookin gooood.

💃

etpinard added 8 commits September 21, 2016 18:21

dragelement: export cover slip creator

a9b83bb

rangeslider: keep ref of input rangeslider container

b45f116

- so that computed rangeslider range can be copied into input container, ensuring that the autorange routine isn't called on updates.

rangeslider: rm (now useless) create_slider.js file

1bb274b

plot api: rm (now useless) range slider setRangeSliderRange

432f599

test: adapt range slider tests

417435f

- with proper class name and full 'translate' values

test: replace data-min / data-max in range slider tests

74607d4

test: add range slider rrelayout test cases

1d7b066

etpinard added bug something broken type: maintenance labels Sep 21, 2016

etpinard added this to the v1.18.0 milestone Sep 21, 2016

etpinard commented Sep 21, 2016

View reviewed changes

test: add a little tolerance for FF

c0ca4f5

mdtusz reviewed Sep 22, 2016

View reviewed changes

rreusser reviewed Sep 22, 2016

View reviewed changes

etpinard added 5 commits October 5, 2016 16:26

test: add toBeWithin jasmine matcher

b062f0b

test: use toBeWithin in range slider test

8502206

move range slider 'range' compute in update

697ed3a

- sometimes the rangeslider range is cleared when no enter selection present, e.g. on relayout(gd, xaxis.rangeslider: { thickness: 0.2 })

book-keep range slider id

b1155a3

rangeslider: inherit 'bgcolor' from 'plot_bgcolor'

2a7696a

etpinard commented Oct 6, 2016

View reviewed changes

etpinard merged commit bedb673 into master Oct 6, 2016

etpinard deleted the range-slider-v2 branch October 6, 2016 16:11

etpinard mentioned this pull request Oct 6, 2016

Range slider second iteration (part 2: on-par range plots !!!) #1017

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Range slider second iteration (part 1) #962

Range slider second iteration (part 1) #962

etpinard commented Sep 21, 2016 •

edited

Loading

etpinard Sep 21, 2016

etpinard Sep 21, 2016

etpinard Sep 21, 2016

etpinard Oct 6, 2016

mdtusz left a comment

mdtusz Sep 22, 2016

etpinard Oct 6, 2016

mdtusz Oct 6, 2016

etpinard Oct 6, 2016

mdtusz Sep 22, 2016

rreusser Sep 22, 2016

mdtusz Sep 22, 2016

rreusser Sep 22, 2016

etpinard Oct 6, 2016

rreusser Oct 6, 2016

mdtusz commented Oct 6, 2016

Range slider second iteration (part 1) #962

Range slider second iteration (part 1) #962

Conversation

etpinard commented Sep 21, 2016 • 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

mdtusz left a 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

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

mdtusz commented Oct 6, 2016

etpinard commented Sep 21, 2016 •

edited

Loading