Add button type to updatemenus #974

rreusser · 2016-09-22T22:34:25Z

This PR adds the following attributes to updatemenus:

type: 'buttons'/'dropdown': if true, is buttons instead of dropdown
direction: 'left'/'right'/'up'/'down': affects which way the buttons extend, though they're still ordered l-to-r or top-to-bottom. It's not inconceivable to auto-detect this when the menu would otherwise go off-screen, but I think less magic is better.
showactive: boolean. Shows active state if true. Set to false to turn the buttons into plain non-stateful buttons.

See live example at: http://rickyreusser.com/animation-experiments/#updatemenus

Notably, this PR does not implement scrolling for overflow. 😞

etpinard · 2016-09-23T15:31:36Z

src/components/updatemenus/attributes.js

+        dflt: false,
+        description: [
+            'For dropdown menus, opens the menu in the reverse direction'
+        ]


👓 missing .join(' ')

Ah, thanks! Yes, noticed that the tests seem to check for that :)

etpinard · 2016-09-23T15:31:56Z

src/components/updatemenus/attributes.js

+        description: [
+            'Determines whether the menu and buttons are laid out vertically',
+            'or horizontally'
+        ]


another missing .join(' ')

etpinard · 2016-09-23T15:34:03Z

src/components/updatemenus/attributes.js

@@ -57,6 +57,37 @@ module.exports = {
        ].join(' ')
    },

+    openreverse: {


I'm not fully in ❤️ with this attribute name, but I can't think of anything else.

Maybe @cldougl could help.

with openreverse: false buttons drop down (or to the right)

with openreverse: true the buttons drop up (or to the left)

I thought about expandreverse.

Does this have to be a boolean (t/f)? My initial thought is that it may be more clear if it's called something with direction or dropdowndirection/expanddirection then have options up / down

For me, the reverse is a tiny bit confusing because I think of the options being in a reversed order rather than the dropdown opening up vs down.

Default is to the right, if horizontal, or down, if vertical. Hence 'reverse' instead of just up/down since the need for left/right would then be modal.

Ahh… maybe orientation should encode both? The problem is that we have two orientations but four directions. What about orientation: 'l'|'r'|'u'|'d' instead of 'h'|'v'. Or direction instead of orientation, if necessary for consistency.

I like the 'l'|'r'|'u'|'d' set of options.

But to avoid confusion, I wouldn't call the attribute orientation (as orientation is already used for legends, bar ...).

Maybe direction: 'l'|'r'|'u'|'d' ?

ah I see! (sorry for the confusion w/ the sideway opening, didn't realize that was an option)

+1 for direction
I like the explicitness of 'l'|'r'|'u'|'d' over true|false

Actually, we spell left and right when specifying text anchors, so let's go with:

direction: 'left'|'right'|'up'|'down'

Changed to direction: 'left'|'right'|'up'|'down'. Waiting for tests to pass.

etpinard · 2016-09-23T15:45:11Z

src/components/updatemenus/draw.js

+                drawButtons(gd, gHeader, gButton, menuOpts);
+            }
+        } else {
+            drawButtons(gd, gHeader, null, menuOpts);


Very lovely way to reuse the existing logic. Thanks!

I hope my code wasn't too hard to understand.

Nope! It was pretty straightforward. Was kinda able to shoehorn my needs into it, as you've seen.

etpinard · 2016-09-23T15:48:39Z

src/components/updatemenus/draw.js

@@ -181,8 +191,8 @@ function drawHeader(gd, gHeader, gButton, menuOpts) {
        .text('▼');


Maybe we could add some logic for that ▼ character around orientation and openreverse.

For example, orientation: 'h' / openreverse: true would put some ◀️ character to the left of the header text. orientation: 'v' / openreverse: ture would put some ⬆️ character etc.

Thought about that. I had mixed feelings. I'm not opposed to it. For comparison, system select menus look like this when near the bottom of the screen.

I have no strong opinion about it.

Let's ask @jackparmer and @delekru

etpinard · 2016-09-23T15:49:21Z

src/components/updatemenus/draw.js

@@ -313,7 +363,7 @@ function styleButtons(buttons, menuOpts) {
    buttons.each(function(buttonOpts, i) {
        var button = d3.select(this);

-        if(i === active) {
+        if(i === active && menuOpts.showactive) {


etpinard · 2016-09-23T15:50:47Z

src/components/updatemenus/draw.js

+
+        // Store per-item sizes since a row of horizontal buttons, for example,
+        // don't all need to be the same width:
+        menuOpts.widths[i] = wEff;


Very nice. I think now this exact same findDimensions routine could be use for RangeSelectors - that will be for a future PR though.

Interesting. Yeah, I had to do some carefully wacky stuff to get it to handle all the cases (h/v, buttons/dropdown, reverse/nonreverse, anchor=t/b/l/r—which is at least no less than like 2^5 = 32 cases), but I think it works…

rreusser · 2016-09-23T17:46:47Z

Fixed image mock (floating point pixel discrepancy; modified for alignment purposes). And added a pretty deluxe new one. Doesn't test opened menus, but I like it anyway. At least sets up difficult-to-mock extras for easy verification in dashboard.

rreusser · 2016-09-23T19:04:51Z

@etpinard the majority of the code went toward positioning so that the image mock tests the majority of what's implemented here. I could add more tests, but I think it'd only be redundant and more code to maintain. Other than that, I think this is ready for review.

For simplicity in reviewing, you can see an update-to-date use that's nearly identical to the mock here: http://rickyreusser.com/animation-experiments/#updatemenus

rreusser · 2016-09-23T22:13:42Z

src/components/updatemenus/draw.js

@@ -43,12 +43,12 @@ module.exports = function draw(gd) {
     *         <g item header />
     *         <text item header-arrow />
     *     <g header-group />
-     *         <g item header />


AHHHH stupid vim plugin. It mangles html tags when you scroll incorrectly… restoring these.

etpinard · 2016-09-26T13:32:41Z

💃 This looks amazing

@rreusser can you update the introduction comment block for future reference to this PR (it's still showing openreverse and orientation). Thanks!

As a side note, I'm kind of kicking myself for not naming the update menu buttons attribute items instead, having { type: 'buttons', buttons: [{ }, /* */] } looks a little weird. But, I guess it's not the end of the world.

etpinard · 2016-09-26T13:34:45Z

Oh and by the way, I added #974 (comment) to #810 - if ever we decide to implement it in the future.

rreusser · 2016-09-26T15:36:06Z

@etpinard yeah, backwards compatibility can be a burden! It's always possible to move to a new syntax and very slowly deprecate the old, but I think this is fine for the foreseeable future.

etpinard added feature something new status: in progress labels Sep 22, 2016

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

etpinard reviewed Sep 23, 2016

View reviewed changes

rreusser added status: reviewable and removed status: in progress labels Sep 23, 2016

rreusser added 10 commits September 23, 2016 18:10

Add button type to updatemenus

39ab95d

Fix a couple updatemenus anchor cases

ddc15fc

Fix minor gapbutton issues

9ee47b1

Fix lint errors and another pixel fix for updatemenus

28fff11

Add showactive attribute to buttons

fb6adc3

Account for arrow padding in updatemenus

65d71d3

Fix updatemenus descriptions and mocks

7f1d35c

Fix updatemenus tests

9a17f69

Add button dropdown tests

68ca4da

Change updatemenus orientation/openreverse to simply 'direction'

f8fba32

rreusser force-pushed the updatemenus-type branch from b6e4653 to f8fba32 Compare September 23, 2016 22:10

rreusser commented Sep 23, 2016

View reviewed changes

Unmangle updatemenus comments

570a448

etpinard mentioned this pull request Sep 26, 2016

Update menus open items #810

Closed

rreusser merged commit 7cc1e6b into master Sep 26, 2016

rreusser deleted the updatemenus-type branch September 26, 2016 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button type to updatemenus #974

Add button type to updatemenus #974

rreusser commented Sep 22, 2016 •

edited

Loading

etpinard Sep 23, 2016

rreusser Sep 23, 2016 •

edited

Loading

etpinard Sep 23, 2016

etpinard Sep 23, 2016

rreusser Sep 23, 2016 •

edited

Loading

cldougl Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

etpinard Sep 23, 2016

cldougl Sep 23, 2016 •

edited

Loading

cldougl Sep 23, 2016

etpinard Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016

etpinard Sep 23, 2016

rreusser Sep 23, 2016

etpinard Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

etpinard Sep 23, 2016

etpinard Sep 23, 2016

etpinard Sep 23, 2016

rreusser Sep 23, 2016

rreusser commented Sep 23, 2016

rreusser commented Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

rreusser commented Sep 26, 2016

		@@ -181,8 +191,8 @@ function drawHeader(gd, gHeader, gButton, menuOpts) {
		.text('▼');

Add button type to updatemenus #974

Add button type to updatemenus #974

Conversation

rreusser commented Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

rreusser Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

cldougl Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

rreusser Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

rreusser Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cldougl Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Sep 23, 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

etpinard Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

rreusser Sep 23, 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

Choose a reason for hiding this comment

rreusser commented Sep 23, 2016

rreusser commented Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard commented Sep 26, 2016

etpinard commented Sep 26, 2016

rreusser commented Sep 26, 2016

rreusser commented Sep 22, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

cldougl Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

cldougl Sep 23, 2016 •

edited

Loading

etpinard Sep 23, 2016 •

edited

Loading

etpinard Sep 23, 2016 •

edited

Loading

rreusser Sep 23, 2016 •

edited

Loading

rreusser commented Sep 23, 2016 •

edited

Loading