scrollable dropdown menus by n-riesco · Pull Request #1214 · plotly/plotly.js

n-riesco · 2016-12-01T18:50:37Z

This PR addresses one of the points in #810 :

provides a helper class ScrollBox (that could be reused to provide scroll boxes elsewhere)
implements scrollable dropdown menus:
- scrollbars are added only if needed
- user can scroll by dragging a scrollbar
- user can scroll by dragging the scrollbox (menu buttons skip the click triggered by dragend)

TODO

Add jasmine tests

EXAMPLE

Here's an example to test the functionality:

// Adapted from https://github.com/plotly/plotly.js/pull/770#issuecomment-234669383

var direction = 'down',
    data = [],
    layout = {
        updatemenus: [{
            direction: direction,
            buttons: [{
                method: 'restyle',
                args: ['line.color', 'red'],
                label: 'red'
            }, {
                method: 'restyle',
                args: ['line.color', 'blue'],
                label: 'blue'
            }, {
                method: 'restyle',
                args: ['line.color', 'green'],
                label: 'green'
            }]
        }, {
            y: 0.7,
            direction: direction,
            buttons: []
        }]
    };

for (var i = 0, n = 20; i < n; i++) {
    data.push({
        y: Array.apply(null, Array(10)).map(() => Math.random()),
        line: {
            shape: 'spline',
            color: 'red'
        },
        visible: i === 0,
        name: 'Data set ' + i,
    });

    var visible = Array.apply(null, Array(n)).map((_, j) => i === j);

    layout.updatemenus[1].buttons.push({
        method: 'restyle',
        args: ['visible', visible],
        label: 'Data set ' + i
    });
}

Plotly.plot('graph', data, layout);

* Don't `Lib.setTranslate` the button container and update the call to `Lib.setTranslate` for each button accordingly. * This change will let us use `Lib.setTranslate` on the button container to implement a scroll box.

etpinard · 2016-12-01T19:59:38Z

Some suggestions:

It would be nice to add a wheelevent scroll handler, similar to how long vertical legends can be scrolled
When clicking on the header, I think the set of dropped buttons should be centred on the active button

etpinard · 2016-12-01T20:02:37Z

src/components/updatemenus/scrollbox.js

+        hbarL = boxL,
+        hbarT = (boxB + hbarH < fullHeight) ? boxB : fullHeight - hbarH;
+
+    var hbar = this.container.selectAll('rect.scrollbar-horizontal').data(


Is this ever going to be implemented?

We made the decision to not allow horizontal scrolling legends. I doubt that dropdown buttons should scroll horizontally in any scenario.

I'd say we can get away with 🔪 this logic.

If we cut horizontal scrollbars, dropdown menus with direction: 'left' | 'right' will be limited to fit within the plot area.

For legends, we could make Scrollbox provide the option to disable the horizontal scrollbar.

etpinard · 2017-01-16T18:57:53Z

@n-riesco are you available to finish off #1214 (comment) in the short term?

* Reduce scroll box minimum size from 44 to 25 pixels. * Allow scroll boxes to use the full width and height.

* Do not assume that only one of the scrollbars is present.

etpinard · 2017-01-30T22:15:43Z

@n-riesco Looking good. This will make a bunch of users happy 🎉

I'm wondering if you noticed that menus that have a scrollbox are a little jumpy on click:

I can help you debug this tomorrow if you like.

n-riesco · 2017-01-30T23:10:27Z

@etpinard I hadn't noticed. I guess that it's caused by removing the clip path before the buttons.

I haven't tested it, but the fix may be as simple as removing this line.

* Changed `ScrollBox#setTranslate(translateX, translateY)` to take pixels. * Previously, invoking `ScrollBox#setTranslate(xf, yf)` would require to access `ScrollBox#_box` to compute `xf` and `yf`. * Now, it's possible to drag the buttons in the scrollbox beyond the scrollbox.

etpinard · 2017-01-31T18:36:02Z

src/components/updatemenus/draw.js

+        if(scrollBox._vbar) {
+            var translateY = 0;
+            for(i = 0; i < active; i++) {
+                translateY += menuOpts.heights[i] + constants.gapButton;


This looks off for direction: 'up' when the updatemenu y isn't the default value:

var direction = 'up', data = [], layout = { updatemenus: [{ y: 0, direction: direction, buttons: [] }] }; for (var i = 0, n = 20; i < n; i++) { data.push({ y: Array.apply(null, Array(10)).map(() => Math.random()), line: { shape: 'spline', color: 'red' }, visible: i === 0, name: 'Data set ' + i, }); var visible = Array.apply(null, Array(n)).map((_, j) => i === j); layout.updatemenus[0].buttons.push({ method: 'restyle', args: ['visible', visible], label: 'Data set ' + i }); } Plotly.plot('graph', data, layout);

gives (after clicking on the header)

Oh! The position of the scrollbox is wrong! This happens in the up direction, when the scrollbox doesn't fit within the chart height. Something similar may happen in the left direction.

This one hasn't been addressed yet, correct?

Looking great now!

etpinard · 2017-01-31T18:45:25Z

src/components/updatemenus/scrollbox.js

+
+    vbar.enter().append('rect')
+        .classed('scrollbar-vertical', true)
+        .call(Color.fill, ScrollBox.barColor);


Would it be ok to chain

.attr('opacity', '0') .transition() .attr('opacity', '1')

here and for hbar to make the scroll bar appear at the same time as the buttons?

Working great now 👍

etpinard · 2017-01-31T18:51:06Z

src/components/updatemenus/scrollbox.js

+ *
+ * @method
+ */
+ScrollBox.prototype.disable = function disable() {


Looks like 9e3ac1b made this method obsolete 🔪

The comment ⬆️ is now obsolete

etpinard · 2017-01-31T19:01:04Z

src/components/updatemenus/scrollbox.js

                .call(onBarDrag);
        }
+
+        this.container.on('wheel', this._onBoxWheel.bind(this));


Currently, the wheel handler isn't called when one tries to scroll with the cursor over the gap between the buttons (you can try increasing the gapButton constant to make this problem easier to detect).

I think the easiest solution would be to make Scrollbox add a background <rect> as big as the clipped <rect> in the container and attach the wheel handler on it.

Working like a charm now. Thanks!

etpinard · 2017-01-31T19:40:42Z

src/components/updatemenus/draw.js

            .attr('opacity', '0')
-            .remove();
+            .remove()
+            .each('end', function() {


This makes the scrollbox disappear after the button exit transition is completed (you can chain .delay(1000) to the transition to make the problem easier to detect).

I would prefer making the scrollbox disappear at the same time as the buttons.

Looking good now 👍

etpinard · 2017-01-31T19:50:58Z

src/components/updatemenus/draw.js

+    var fullLayout = gd._fullLayout,
+        scrollBoxId = 'updatemenus' + fullLayout._uid + menuOpts._index,
+        scrollBoxPosition = {
+            l: menuOpts.lx + menuOpts.borderwidth + x0 + menuOpts.pad.l,


Nice job taking into account borderwidth 🎉

* Previously, the scrollbox was created every time the dropmenu was unfolded. * Now, the scrollbox is created right after creating the dropdown container. * Moved the logic to fold and unfold the dropdown menu to functions foldDropdownMenu and unfoldDropdownMenu. * Moved all the logic to compute the position and size of the scrollbox to function unfoldDropdownMenu. * Moved logic to disable the scrollbox to function foldDropdownMenu. * This refactor will help introduce a background <rect> element in the dropdown container before any dropdown buttons. * It will also help introduce a transition to show and hide the scrollbars.

* Added a <rect> background to the scrollbox, so that events on the gaps between buttons aren't dropped.

* Fixes issues caused by the relayout tests, where `updatemenus[1]` ended up without a buttons field. * A transition has been added to show and hide the scroll bars.

* Ensure scrollbox size is recomputed every time, drawButtons is called.

* Account for the direction of the scrollbox to determine to which side the scrollbox will be anchored.

etpinard · 2017-02-03T15:13:30Z

Great @n-riesco looks like all the functionality is 👌

Ping me once you fixed the merge conflict and are happy with your test cases.

Awesome work 👍

* Fixed regexp in Lib.getTranslate to accept negative values.

* Conflicts: src/components/updatemenus/draw.js src/components/updatemenus/scrollbox.js src/lib/index.js test/jasmine/tests/cartesian_test.js test/jasmine/tests/updatemenus_test.js

n-riesco · 2017-02-03T18:21:42Z

@etpinard Ready for review!

This PR also fixes Drawing.getTranslate (the previous regexp drops - signs).

etpinard · 2017-02-03T19:00:50Z

test/jasmine/tests/updatemenus_test.js

 });
+
+
+describe('update menus interaction with scrollbox:', function() {


💯 / 💯 tests. Thanks!

etpinard · 2017-02-03T19:05:20Z

src/lib/index.js

 lib.getTranslate = function(element) {

-    var re = /.*\btranslate\((\d*\.?\d*)[^\d]*(\d*\.?\d*)[^\d].*/,
+    var re = /.*\btranslate\((-?\d*\.?\d*)[^-\d]*(-?\d*\.?\d*)[^\d].*/,


Nice catch 🔬

Would you mind adding a test case in drawing_test.js to 🔒 this down?

(No rush on this by the way. If you don't have the time to do this by Monday night, I'll merge your PR as is and open up an issue about this).

etpinard · 2017-02-03T19:06:52Z

#1214 (comment) is non-blocking.

This PR will be merged and released as part of 1.23.0 on Feb 6.

etpinard · 2017-02-06T14:56:48Z

Merging. Thanks @n-riesco 🎉

ayh9kim · 2017-02-07T16:19:55Z

Will this work for Ropensci/plotly automatically?

n-riesco added 2 commits December 1, 2016 10:29

updatemenus: don't setTranslate button container

2ec79e7

* Don't `Lib.setTranslate` the button container and update the call to `Lib.setTranslate` for each button accordingly. * This change will let us use `Lib.setTranslate` on the button container to implement a scroll box.

updatemenus: add scroll bars if needed

5a4ecf3

etpinard added status: in progress feature something new labels Dec 1, 2016

etpinard reviewed Dec 1, 2016

View reviewed changes

n-riesco added 7 commits January 26, 2017 14:37

updatemenus: Fix image test failure

66e503c

* Reduce scroll box minimum size from 44 to 25 pixels. * Allow scroll boxes to use the full width and height.

Merge branch 'master' into pr-20161121-scrollable-dropdown-menus

9a0aa64

updatemenus: Update copyright notice

8df8fd6

updatemenus: make ScrollBox#setTranslate public

7057bc7

updatemenus: fix positioning of scrollbars

8d1f333

* Do not assume that only one of the scrollbars is present.

updatemenus: center dropmenu on active option

563009b

updatemenus: hide scrollbar if header clicked

ef5210e

etpinard added this to the 1.23.0 milestone Jan 30, 2017

n-riesco added 3 commits January 31, 2017 12:52

updatemenus: fix smooth dropdown folding

9e3ac1b

updatemenus: handle mouse wheel events

13508da

etpinard reviewed Jan 31, 2017

View reviewed changes

n-riesco added 4 commits February 1, 2017 18:19

updatemenus: add <rect> background to scrollbox

4975401

* Added a <rect> background to the scrollbox, so that events on the gaps between buttons aren't dropped.

updatemenus: remove un/foldDropdownMenu

3b17d1d

* Fixes issues caused by the relayout tests, where `updatemenus[1]` ended up without a buttons field. * A transition has been added to show and hide the scroll bars.

updatemenu: fix computation of scrollbox size

b155a8a

* Ensure scrollbox size is recomputed every time, drawButtons is called.

updatemenus: fix positioning of scrollbox

109f284

* Account for the direction of the scrollbox to determine to which side the scrollbox will be anchored.

n-riesco added 3 commits February 3, 2017 16:00

Lib: Fix regexp in getTranslate

e7c3ae3

* Fixed regexp in Lib.getTranslate to accept negative values.

updatemenus: test scrollbox

89c615d

Merge branch 'master' into PR plotly#1214

a554ea6

* Conflicts: src/components/updatemenus/draw.js src/components/updatemenus/scrollbox.js src/lib/index.js test/jasmine/tests/cartesian_test.js test/jasmine/tests/updatemenus_test.js

n-riesco changed the title ~~[WIP] scrollable dropdown menus~~ scrollable dropdown menus Feb 3, 2017

etpinard reviewed Feb 3, 2017

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Feb 3, 2017

Drawing: test setTranslate works with negatives

79f1107

etpinard merged commit 3fad104 into plotly:master Feb 6, 2017

etpinard mentioned this pull request Feb 8, 2017

Update menus open items #810

Closed

alexcjohnson mentioned this pull request Jul 6, 2017

[WIP] Table view #1834

Closed

		});


		describe('update menus interaction with scrollbox:', function() {

Uh oh!

Conversation

n-riesco commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etpinard commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Jan 16, 2017

Uh oh!

etpinard commented Jan 30, 2017

Uh oh!

n-riesco commented Jan 30, 2017

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 Jan 31, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Feb 3, 2017

Uh oh!

n-riesco commented Feb 3, 2017 • 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!

etpinard commented Feb 3, 2017

Uh oh!

etpinard commented Feb 6, 2017

Uh oh!

ayh9kim commented Feb 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

n-riesco commented Dec 1, 2016 •

edited

Loading

etpinard Jan 31, 2017 •

edited

Loading

n-riesco commented Feb 3, 2017 •

edited

Loading