Slider component by rreusser · Pull Request #986 · plotly/plotly.js

rreusser · 2016-09-28T06:54:00Z

This PR implements a first cut at sliders #742
cc @etpinard

It has most of the necessary features (including hooking into the internal event dispatcher 🎉) and seems reasonably stable. It needs:

design feedback
should echo the selected value since the label may get dropped if there's not enough room
should expose more configuration options
perhaps account for continuous sliders out of the gate so we don't work ourselves into a backwards compatibility corner
fix circular deps… orrrr raise allowed circular deps 😏
make the constants less intelligent and more flexible. Just spell them out. Maybe grab tick constants from elsewhere.
tests 😬

Notable features:

measures text and sets major/minor tick stride to avoid label overlap
subscribes to events and updates accordingly
wooooo transitions

etpinard · 2016-09-28T13:59:28Z

@rreusser can you share a few screenshots here for the design team?

rreusser · 2016-09-28T14:02:58Z

Note for design-oriented eyes: As mentioned above, a big thing that this currently needs is a readout of the current value, as in "Year: 1980". The best place is probably above or below the slider (not next to it!) because variable-length user input is a mess when there's not enough room for it. Thoughts welcome.

Here's the slider at full size. It can be translated, but vertical orientation is not currently implemented. The play button is currently drawn next to it, but there is no general relation between the slider and buttons.

Here it is at reduced size. The label size is calculated and it drops labels so that they don't overlap.

You can also achieve other looks by messing with the constants. They're not currently editable by end-users, but they certainly could be:

Ticks too. Just throwing out possibilities here…

rreusser · 2016-09-28T14:47:54Z

@etpinard Added mock. The big shortcoming is that it doesn't hook up to events since mocks don't work with animations.

rreusser · 2016-09-28T14:55:41Z

src/components/slider/draw.js

+
+function makeInputProxy(gd, sliderGroup, sliderOpts) {
+    sliderOpts.inputProxy = gd._fullLayout._paperdiv.selectAll('input.' + constants.inputProxyClass)
+        .data([0]);


This is not currently implemented. The idea is that it creates an <input type="range"> that captures focus (can it actually do that programatically?) so that accessibility things like keyboard arrows to change the slider position actually work. Just an idea. Might need to go.

Thanks for the info

rreusser · 2016-09-28T14:57:01Z

src/components/slider/attributes.js

+        ].join(' ')
+    },
+
+    transition: {


I kept this nested format so that it perfectly mirrors the way you'd define transition config for an animation. Otherwise it would need to be transitionduration: and transitioneasing:, which isn't bad. Open to thoughts here.

👍 plotly.js is a big fan of nested attributes.

rreusser · 2016-09-28T14:57:48Z

src/components/slider/draw.js

+    sliders.exit().remove();
+
+    // If no more sliders, clear the margisn:
+    if(sliders.exit().size()) clearPushMargins(gd);


When sliders disappear, it also needs to detach event subscriptions. That's very important! We should think about the proper way for components to do that in general.

Good point.

rreusser · 2016-09-28T14:59:34Z

src/components/slider/draw.js

+    sliderOpts.inputAreaLength = Math.round(sliderOpts.outerLength - sliderOpts.xpad * 2);
+    sliderOpts.railInset = Math.round(Math.max(0, constants.gripWidth - constants.railWidth) * 0.5);
+    sliderOpts.stepInset = Math.round(Math.max(sliderOpts.railInset, constants.gripWidth * 0.5));
+


The logic in this area needs to be 30% less fancy and 40% more directly controlled by constants.

etpinard

@rreusser this is looking great

The main issue I found so far was a naming inconsistency slider vs sliders. The slider directory should be named src/components/sliders/ consistent with layout.sliders attribute container.

etpinard · 2016-09-28T14:00:37Z

src/components/slider/attributes.js

+        dflt: 'restyle',
+        role: 'info',
+        description: [
+            'Sets the Plotly method to be called on click.'


s/on click/on slide maybe?

etpinard · 2016-09-28T14:01:27Z

src/components/slider/attributes.js

+
+    steps: stepsAttrs,
+
+    updateevent: {


etpinard · 2016-09-28T14:06:39Z

src/components/slider/attributes.js

+            'value when an event of type `updateevent` is received. If',
+            'undefined, the data argument itself is used. If a string,',
+            'that property is used, and if a string with dots, e.g.',
+            '`item.0.label`, then `data[\'item\'][0][\'label\']` will',


Ok. Plotly.restyle and Plotly.relayout use use [] to delimit array items in an attribute string e.g. item[0].label via Lib.nestedProperty .

I believe the item.0.label is a little more clear. But, maybe we should stick to item[0].label for consistency?

etpinard · 2016-09-28T14:08:16Z

src/components/slider/attributes.js

+        role: 'info',
+        dflt: 'fraction',
+        description: [
+            'Determines whether this color bar\'s length',


Haha I see, the descriptions were pasted over from other components.

Hahaha yupppp.

etpinard · 2016-09-28T14:17:57Z

src/components/slider/draw.js

+        return function(data) {
+            var value = data;
+            if(updatevalue) {
+                value = Lib.nestedProperty(data, updatevalue).get();


wait does nestedProperty work with items.0.label signature?

maybe I just didn't know about it.

Ah, that's a good question… It may well not. I simply assumed (hoped?) it would. Thanks for noticing.

etpinard · 2016-09-28T14:21:39Z

src/components/updatemenus/draw.js


 // find item dimensions (this mutates menuOpts)
-function findDimenstions(gd, menuOpts) {
+function findDimensions(gd, menuOpts) {


Haha, I love finding typos and then finding the compensatory typos elsewhere…

etpinard · 2016-09-28T14:22:15Z

src/components/slider/index.js

+
+exports.moduleType = 'component';
+
+exports.name = 'slider';


plural (to be consistent with layout.sliders

etpinard · 2016-09-28T14:26:25Z

src/components/slider/draw.js

+    });
+}
+
+function drawTicks(sliderGroup, sliderOpts) {


Good enough for now i.e. no need to make the slider tick options on par with axis tick options (++ axes.js is most fun module to reuse).

EDIT: (++ axes.js is not the most fun module to reuse)

I didn't directly use axes.js since the options didn't perfectly map 1-1, but followed the naming conventions.

etpinard · 2016-09-28T14:36:24Z

src/components/slider/draw.js

+    item.on('mousedown', function() {
+        var grip = sliderGroup.select('.' + constants.gripRectClass);
+
+        d3.event.stopPropagation();


we might want to use a coverSlip in the range slider part II here to avoid selecting other elements on the graph on drag.

Ah but looks like d3.event.stopPropagation(); d3.event.preventDefault(); does the trick. Nevermind.

Yeah, I wasn't 100% sure about my ad-hoc drag implementation. I feel like it ended up fairly concise, unsurprising, and reasonably robust, but I'm glad to refactor if there are helpers that manage some of this.

etpinard · 2016-09-28T14:37:14Z

src/components/slider/draw.js

+
+    // Lib.setTranslate doesn't work here becasue of the transition duck-typing.
+    // It's also not necessary because there are no other transitions to preserve.
+    el.attr('transform', 'translate(' + (x - constants.gripWidth * 0.5) + ',' + 0 + ')');


thanks for the comment

etpinard · 2016-09-28T15:06:48Z

@jackparmer @delekru

would mind taking a look at #986 (comment) ?

rreusser · 2016-09-28T15:12:04Z

@etpinard thoughts on decircularizing requires vs. upping the circularity tolerance threshold?

etpinard · 2016-09-28T15:17:53Z

thoughts on decircularizing requires vs. upping the cirularity threshold?

Every component that calls a Plotly method will have a circular dependency in the current state of the src/ folder. So, your PR should indeed add another circular dependency.

That test-syntax block is simply there to be aware that at some point var MAX_ALLOWED_CIRCULAR_DEPS = 0;.

So, go ahead and bump MAX_ALLOWED_CIRCULAR_DEPS !

rreusser · 2016-09-28T15:19:11Z

Thanks @etpinard. Will gladly bump threshold!

jackparmer · 2016-09-28T23:47:29Z

Love these sliders. 💃 from me on the design side.

add streaming maxpoints max and dflt

rreusser · 2016-10-06T14:14:08Z

@etpinard I've removed updateevent and updatevalue and cleaned up the slider options to more or less conform to axis options. The configurable options are now:

font
bgcolor
bordercolor
borderwidth
activebgcolor

ticklen
tickwidth
tickcolor
minorticklen

transition.duration
transition.easing

currentvalue.visible
currentvalue.xanchor
currentvalue.prefix
currentvalue.offset
currentvalue.font

The baseline is now within the range of what can be styled:

etpinard

@rreusser looking good!

In this review, there's:

1 blocking comment about a slight defaults quirk.
1 question about the slider coerce('bgcolor') call
1 possible test enhancement.

etpinard · 2016-10-06T15:22:32Z

src/components/sliders/attributes.js

        description: 'Sets the width (in px) of the border enclosing the slider.'
-    }
+    },
+    ticklen: {


etpinard · 2016-10-06T15:24:15Z

src/components/sliders/defaults.js

-    Lib.coerceFont(coerce, 'currentvalue.font', layoutOut.font);
-
-    coerce('bgcolor', layoutOut.paper_bgcolor);
+    coerce('bgcolor');


I see the default bgcolor isn't inherited from layoutOut.paper_bgcolor anymore. Is this on purpose?

No good reason except that it didn't actually seem like a desirable default since that would just make the slider transparent-looking instead of filled by default.

To elaborate slightly, this attribute wasn't used before. Until today it was a hard-coded constant.

it didn't actually seem like a desirable default

I agree 100%. I just wanted to make sure the above patch was not a typo 👍

etpinard · 2016-10-06T15:27:30Z

src/components/sliders/attributes.js

+
+        xanchor: {
+            valType: 'enumerated',
+            values: ['left', 'center', 'right'],


yep, nice job dropping the 'auto' value here.

etpinard · 2016-10-06T15:29:06Z

src/components/sliders/attributes.js

+            ].join(' ')
+        },
+
+        prefix: {


We should probably add a suffix attribute too.

Sounds good. Good idea.

etpinard · 2016-10-06T15:33:52Z

src/components/sliders/defaults.js

+    coerce('currentvalue.visible');
+    coerce('currentvalue.xanchor');
+    coerce('currentvalue.prefix');
+    coerce('currentvalue.offset');


this should be:

var currentValue = coerce('currentvalue.visible'); if(currentValue) { coerce('currentvalue.xanchor'); coerce('currentvalue.prefix'); coerce('currentvalue.offset'); }

so that Plotly.validate can detect e.g.

layout.sliders = [{ currentvalue: { visible: false, xanchor: 'left' }]; // => `xanchor` has no effects

etpinard · 2016-10-06T15:34:55Z

src/components/sliders/defaults.js

+    coerce('minorticklen');
+
+    Lib.coerceFont(coerce, 'font', layoutOut.font);
+    Lib.coerceFont(coerce, 'currentvalue.font', sliderOut.font);


similarly here, Lib.coerceFont(coerce, 'currentvalue.font', sliderOut.font); should only get coerced if currentvalue.visible is true.

etpinard · 2016-10-06T15:36:41Z

test/image/mocks/sliders.json

@@ -0,0 +1,172 @@
+{
+  "data": [


great mock! Thanks 🎉

Goal: hit as many properties as possible to avoid testing styles in jasmine.

etpinard · 2016-10-06T15:37:07Z

test/jasmine/assets/fail_test.js

        expect(error).toBeUndefined();
    }
+    if(error && error.stack) {
+        console.error(error.stack);


nicely done 🎉

etpinard · 2016-10-06T15:38:23Z

test/jasmine/tests/sliders_test.js

+var destroyGraphDiv = require('../assets/destroy_graph_div');
+var fail = require('../assets/fail_test');
+
+describe('sliders defaults', function() {


TODO:

add one defaults around currentvalue.visible -> other currentvalue nested attributes

etpinard · 2016-10-06T15:41:35Z

test/jasmine/tests/sliders_test.js

+    });
+});
+
+describe('update sliders interactions', function() {


Have you thought about adding mouse interaction tests?

It should be doable to mock a drag interaction using new MouseEvent - unless there are complications that I can't think about.

etpinard

@rreusser looking good!

In this review, there's:

1 blocking comment about a slight defaults quirk.
1 question about the slider coerce('bgcolor') call
1 possible test enhancement.

rreusser · 2016-10-06T17:12:14Z

On a minor note, I precomputed the size of the text labels so that I could keep the prefix in the same place for right-anchored currentvalue output. The rationale is it makes it more clear which part is the value with the common use case of "label: value". I have mixed feelings on this.

rreusser · 2016-10-06T18:31:02Z

@etpinard I've made the requested changes:

add suffix 70610ec
fix coerce when currentvalue !visible f25127c, 4b608ee
test mouse events (down, move, up) and associated appearance changes 6c9034d

etpinard · 2016-10-06T19:10:26Z

test/jasmine/tests/sliders_test.js

+            expect(mockCopy.layout.sliders[0].active).toEqual(0);
+
+            done();
+        }, 100);


Very nice. That's all I wanted. Thanks for taking the time to do this 🎉

No prob. Wasn't too bad; just hadn't previously tried mocking mouse events and positions.

etpinard · 2016-10-06T19:11:29Z

I precomputed the size of the text labels so that I could keep the prefix in the same place for right-anchored currentvalue output. The rationale is it makes it more clear which part is the value with the common use case of "label: value".

I have no strong opinion about this. I'll you decide what's best.

rreusser · 2016-10-06T19:14:44Z

I think it's fine as-is. I thought it would be preferable to completely right-justifying the text it since this cuts down on unneeded movement as you change the slider.

etpinard · 2016-10-06T19:24:51Z

@rreusser I'll attach the gif here for reference:

looks like some race condition exist makes the currentvalue position change on relayout.

That happen once and I can't replicate it though.

So, let's merge this in 💃 whenever you're happy with it.

rreusser · 2016-10-06T19:39:09Z

Because it's particularly evasive (race condition?), @etpinard noted a layout issue that's captured here:

I think this was an issue with bounding box computation returning null and the sizes not actually being numbers. The last commit on this PR should fix that, but just adding here for reference in case it pops up again.

rreusser added 2 commits September 27, 2016 23:56

First cut at slider

f5bccd5

Fix lint errors in slider

83483fd

rreusser added status: in progress labels Sep 28, 2016

rreusser added 2 commits September 28, 2016 03:35

Add slider transitions

f69a328

Expand touchable slider area

82ea558

etpinard added the feature something new label Sep 28, 2016

Add slider mock

ca5f9f5

rreusser commented Sep 28, 2016

View reviewed changes

etpinard suggested changes Sep 28, 2016

View reviewed changes

rreusser added 5 commits September 28, 2016 11:57

Rename sliders and bump circular require tolerance

c6a3e11

Add trbl padding to sliders

5b74652

Remove fanciness from slider constants

e13e0ee

Tweak slider behavior to avoid event -> method loops

8fe1842

Improve padding logic for updatemenus

05ac586

rreusser and others added 2 commits September 29, 2016 08:52

Add jasmine tests for updatemenus padding

4604341

add streaming maxpoints max and dflt

8558be8

etpinard mentioned this pull request Sep 29, 2016

Unify component pad logic #997

Closed

Merge pull request #996 from plotly/maxpt_dflt

c673afe

add streaming maxpoints max and dflt

rreusser added 3 commits October 3, 2016 15:36

Test updateevent for sliders

4775a47

Update sliders baseline to reflect bugfix

a08f084

Fix plot schema error

5816c24

rreusser mentioned this pull request Oct 4, 2016

Bindings #1002

Closed

rreusser added 4 commits October 4, 2016 14:45

Remove unneeded lines from sliders test

2cfa81b

Remove updatevalue and updateevent

1d52b51

Removed unused slider function

892583d

Clean up slider options

5015710

Remove unneeded/failing slider test

582c643

rreusser added status: reviewable and removed status: in progress labels Oct 6, 2016

rreusser mentioned this pull request Oct 6, 2016

Common interface to interpret and execute API methods #1016

Merged

3 tasks

etpinard suggested changes Oct 6, 2016

View reviewed changes

rreusser added 4 commits October 6, 2016 13:14

Add suffix to currentvalue slider label

70610ec

don't coerce slider currentvalue attrs when !visible

f25127c

Be more specific about *all* unused slider attrs not set

4b608ee

Test mouse movement for sliders

6c9034d

etpinard reviewed Oct 6, 2016

View reviewed changes

etpinard approved these changes Oct 6, 2016

View reviewed changes

Make sure slider Drawing.bBox calls fall back to defined

21e2f99

rreusser merged commit cb293c7 into master Oct 6, 2016

rreusser deleted the slider-component branch October 6, 2016 19:59


		exports.moduleType = 'component';

		exports.name = 'slider';

Uh oh!

Conversation

rreusser commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etpinard commented Sep 28, 2016

Uh oh!

rreusser commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rreusser commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Sep 28, 2016

Uh oh!

rreusser commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etpinard commented Sep 28, 2016

rreusser commented Sep 28, 2016 •

edited

Loading

rreusser commented Sep 28, 2016 •

edited

Loading

rreusser commented Sep 28, 2016 •

edited

Loading

etpinard Sep 28, 2016 •

edited

Loading

rreusser Sep 28, 2016 •

edited

Loading

rreusser commented Sep 28, 2016 •

edited

Loading