Add config options to remove specific modebar buttons #23

etpinard · 2015-11-20T22:26:16Z

A lot of folks have asked for ways to remove specify modebar buttons.

This PR:

Refactors a good chunk of the modebar code. Modebar instances now have an update method able to update the buttons and logo based on the context.
Moves Modebar management out of Fx object and into its own file.
Adds a config options tentatively named modebarButtonsToRemove (suggestions are welcomed) which takes in an array of modebar buttons names to be removed.
Remove modebar buttons click handler away from Modebar constructor. The Modebar constructor now expects buttons config objects, fully describing them. This puts plotly.js default buttons on-par with custom modebar buttons.
Adds a bunch of modebar tests.

- 'click' keys are now linked to strings corresponding to the name of the Modebar prototype method instead of being linked to the prototype methods themselves. - 'val' can be a function of graphInfo (e.g. for 'hoverCompare2d')

- rm !(this instanceof ModeBar) code path - rename 'config' arg --> 'opts' to not be mistaken with the buttons config

instead of relying on internal Plotly.

- manageModebar handles logic around which button should appear on the modebar and how to modebar buttons are grouped.

- use it in manageModebar to update buttons based on plot type and config arguments

- no need to remove entire modebar element when buttons need to be changed. - remove only child elements

- adds ability to remove specific modebar buttons by listing their names upon plot configuration - throw error if 'modebarButtonsToRemove' isn't an Array - add filter step to choose button routine, removing buttons that are in 'modebarButtonsToRemove' list.

etpinard · 2015-11-20T22:28:30Z

src/components/modebar/buttons_config.js

+        group: 'ext',
+        title: 'download plot as a png',
+        icon: 'camera',
+        click: 'toImage'


i.e. the name of the associated Modebar method.

This seems overly declarative to me, almost like we intend to ship this config over the wire. Having the click handler attached to this structure was simpler. What are the benefits in associating a function to a button via a string?

What are the benefits in associating a function to a button via a string?

Putting the config in a separate file.

i.e. readability.

I think separate files are less readable actually. If it was a 1000 line structure I would be open to breaking it up. But this seems to add more cognitive dissonance

What about dry-ness? The list of buttons groups was repeated in three places before. Now only once.

- click handler are function of (modabar, eventObject)

- each button's config is passed to the constructor on init.

- the manager main purpose now is to pass the appropriate button groups and configs to the modebar constructor depending on the plot type.

- throw error when name is not passed in - throw error when name is not unique - check for button 'name' is Modebar.hasButton method

- add fallbacks to 'title', and 'toggle' and 'icon' - throw error when 'click' handler isn't given

etpinard · 2015-11-23T21:49:00Z

src/components/modebar/buttons.js

+var Snapshot = require('../../snapshot');
+
+
+var modebarButtons = module.exports = {};


@bpostlethwaite default plotly.js modebar button config objects (that now include click handler definitions) are defined here.

i.e the Modebar constructor expects the button config objects as input, but doesn't know anything about them a priori.

alexcjohnson · 2015-11-24T15:27:09Z

I'd vote for Modebar, modebar and modebarButtons (although displayModeBar will need to stay for backward compat.)

I suppose we could support both displayModeBar and displayModebar if we want to change to lowercase b, and warn devs about the deprecated uppercase B. But unless you think lowercase b is more intuitive (I don't, modebar isn't one word, it's two) I'd still vote for uppercase B.

etpinard · 2015-11-24T15:59:44Z

(I don't, modebar isn't one word, it's two) I'd still vote for uppercase B

@alexcjohnson that convinces me, ModeBar and modeBar it is.

etpinard · 2015-11-24T16:02:08Z

Their custom buttons would go at the end, after the built-in buttons. I don't think most users would mind this. That's a pretty normal pattern I think for extending built-in menu bars and the like.

That's a valid point. Users wanting to simply add a buttons would use modeBarButtonsToAdd and users wanting full control would use modeBarButtons. I don't see a problem with supporting both.

- modebar click handler are now functions of (gd, ev) the graph div and the event object only. - remove all traces of modebar methods inside click handler - call updateActiveButtons after click handler - N.B. updateActiveButtons is also called in Plotly.relayout

so that 'dragmode' update also go through the modebar manager

- don't show hover text when 'title' is set to null or false or '' - if undefined, fall back to 'name'

bpostlethwaite · 2015-11-24T18:25:58Z

@etpinard if we are not putting 'add button' functionality in this go around could you make an issue and reference it in this PR? That way we can review the discussions here to refresh ourselves once they do go in

Plotly.Icons has the full set of icons, ready to be used by users in their custom modebar buttons

- takes an array of buttons config objects (as the defined for the default buttons in modebar/buttons.js) - these 'buttons to add' get appended an additional group of buttons passed to the ModeBar constructor

- this allows for fully custom modebar buttons - modeBarButtons is a nested array of button config objects - support 'string' entries to reference default buttons

etpinard · 2015-11-24T20:37:32Z

src/components/modebar/index.js

-    button.addEventListener('click', function () {
-            config.click.apply(_this, arguments);
+    var title = config.title;
+    if(title !== null && title !== false && title !== '') {


@alexcjohnson OK with this logic? It felt weird handling null and '' but not false.

good point. Could also take anything falsey except undefined as "no title," ie if(title || title === undefined) - actually, what do we want to happen if title is 0? I can imagine people getting confused results from that, like if someone makes a set of buttons that they want to label 0, 1, 2, 3, but the first one comes out labeled "setParamCount0" or something... strange, but we might as well plan for it, something like:

var title = config.title; if(title === undefined) title = config.name; if(title || title === 0) button.setAttribute('data-title', title);

sounds good

alexcjohnson · 2015-11-25T10:03:43Z

src/plotly.js

@@ -25,8 +25,7 @@ exports.Lib = require('./lib');
 exports.util = require('./lib/svg_text_utils');
 exports.Queue = require('./lib/queue');

-// plot icons svg and plot css
-exports.Icons = require('../build/ploticon');


What if users want to use our icons for their custom buttons?

That's here: https://github.com/plotly/plotly.js/pull/23/files#diff-1fdf421c05c1140f6d71444ea2b27638R36

Ah right - I forgot we had both of these... 👍

alexcjohnson · 2015-11-25T10:12:31Z

just 2 non-blocking comments left from me - 💃

Add config options to remove specific modebar buttons

harish2rb · 2016-01-30T00:17:05Z

To all the gurus here, is it possible to use 'modeBarButtonToAdd' in plotly R yet. I am not able to get it working in R. Any help is appreciated.

etpinard added 14 commits November 20, 2015 17:18

clean up ModeBar API:

84a7c06

- rm !(this instanceof ModeBar) code path - rename 'config' arg --> 'opts' to not be mistaken with the buttons config

require Icons in modebar index directly,

387e7da

instead of relying on internal Plotly.

rename modebar_config -> buttons_config

079f747

mv create/update Modebar wrapper in modebar/ :

189e313

- manageModebar handles logic around which button should appear on the modebar and how to modebar buttons are grouped.

make main modebar module export a creator function

a320f29

add update method to Modebar:

22a6f3b

- use it in manageModebar to update buttons based on plot type and config arguments

replace Modebar.cleanup with removeAllButtons:

18e09dc

- no need to remove entire modebar element when buttons need to be changed. - remove only child elements

update Modebar.hasButtons for cases where buttons haven't been drawn yet

9a3e45b

add Modebar.destroy method

10d591b

generalize findButtons helper to look for buttons by category and group

a79aba3

lint

aac197e

update / add modebar jasmine tests

b826ddf

etpinard reviewed Nov 20, 2015
View reviewed changes

etpinard added 6 commits November 23, 2015 16:36

add click handler definition in modebar button config:

b3fdb5d

- click handler are function of (modabar, eventObject)

make Modebar constructor not rely on button config in scope:

74350d6

- each button's config is passed to the constructor on init.

update modebar manager:

da787d7

- the manager main purpose now is to pass the appropriate button groups and configs to the modebar constructor depending on the plot type.

add check for button 'name':

bb3fb10

- throw error when name is not passed in - throw error when name is not unique - check for button 'name' is Modebar.hasButton method

robustify button config API:

933603c

- add fallbacks to 'title', and 'toggle' and 'icon' - throw error when 'click' handler isn't given

update modebar tests

543fe14

etpinard reviewed Nov 23, 2015
View reviewed changes

etpinard added 6 commits November 24, 2015 12:07

merge doSceneDragmode and domodebar relayout checks,

5c74610

so that 'dragmode' update also go through the modebar manager

add full svg Icons to modebar button config

ef3c734

make svg 'ascent' and 'descent' props part of every plotly icons

7cd5b30

rename 'graphInfo' --> 'gd' for consistency in modebar buttons defs

2934f11

improve fallback for button 'title':

eff56fc

- don't show hover text when 'title' is set to null or false or '' - if undefined, fall back to 'name'

etpinard added 8 commits November 24, 2015 13:35

sub 'modebar' --> 'modeBar'

ed90f81

export Icons,

9972341

Plotly.Icons has the full set of icons, ready to be used by users in their custom modebar buttons

add 'modeBarButtonsToAdd' config arg:

313da5c

- takes an array of buttons config objects (as the defined for the default buttons in modebar/buttons.js) - these 'buttons to add' get appended an additional group of buttons passed to the ModeBar constructor

add 'modeBarButtons' config arg:

de960ab

- this allows for fully custom modebar buttons - modeBarButtons is a nested array of button config objects - support 'string' entries to reference default buttons

Merge branch 'master' into custom-modebar

897e309

make Icons.question the fall back icon

08843ef

fix condition on custom buttons

4e4afc6

add test for modeBar 'add' and 'fully custom' modebar buttons

3dd0c15

etpinard reviewed Nov 24, 2015
View reviewed changes

improve logic around button title and name

71033b9

alexcjohnson reviewed Nov 25, 2015
View reviewed changes

etpinard added 2 commits November 25, 2015 09:38

add ability to multiple mode bar button groups via 'modeBarButtonToAdd'

26936aa

add comments

eadd7fb

etpinard added a commit that referenced this pull request Nov 25, 2015

Merge pull request #23 from plotly/custom-modebar

2165998

Add config options to remove specific modebar buttons

etpinard merged commit 2165998 into master Nov 25, 2015

etpinard deleted the custom-modebar branch November 25, 2015 14:57

		var Snapshot = require('../../snapshot');


		var modebarButtons = module.exports = {};

Uh oh!

Add config options to remove specific modebar buttons #23

Add config options to remove specific modebar buttons #23

Uh oh!

Conversation

etpinard commented Nov 20, 2015

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!

alexcjohnson commented Nov 24, 2015

Uh oh!

etpinard commented Nov 24, 2015

Uh oh!

etpinard commented Nov 24, 2015

Uh oh!

bpostlethwaite commented Nov 24, 2015

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!

alexcjohnson commented Nov 25, 2015

Uh oh!

harish2rb commented Jan 30, 2016

Uh oh!

Uh oh!