Reduce boilerplate: allow to define actions with options object

[yohanboniface]

Note: this is a suggestion for discussion. :)

While trying to switch uMap to using Leaflet.Toolbar, I found myself with quite a lot of repetitive code, like defining a new class for every action, then giving this new class as parameter to the toolbar.

I think a nice API should be very very simple on the first approach, and then allow more complex needs by more complex calls. And certainly it's often that our actions are only a className, a tooltip and a callback.

So this PR aim to allow creating actions this way:

new L.Toolbar({ actions: [
    {
        tooltip: 'Back to center',
        className: 'custom-class',
        onEnable: myCallback
    },
    {
        tooltip: 'Back to center2',
        className: 'custom-class2',
        onEnable: myCallback2
    }
] });

instead of:

MyNewAction = L.ToolbarAction.extends({
    options: {
        toolbarIcon: {
            className: 'custom-class',
            tooltip: 'Back to center'
        }
     },

    addHooks: function () {
        myCallback();
    }
});

MyNewAction2 = L.ToolbarAction.extends({
    options: {
        toolbarIcon: {
            className: 'custom-class2',
            tooltip: 'Back to center2'
        }
     },

    addHooks: function () {
        myCallback2();
    }
});

new Toolbar({actions: [MyNewAction, MyNewAction2]});

This includes:

• allowing to give only options instead of a class as Toolbar.options.actions parameter
• remove toolbarIcon options key: as we have a one to one relation between the action and the icon, and we don't have a html key for the action and one for the icon for example, it's safe and much simpler to only deal with one level object
• have a default addHooks / removeHooks that listen to options.onEnable / options.onDisable optional keys

Of course, one can still make its own ToolbarAction class and do whatever he wants. My goal here is only to make the library first step much simpler.

Thanks in advance for your feedback! :)

[coveralls]

Coverage increased (+1.6%) to 95.2% when pulling 4beeae41a70aa22cd278e3f9d5bae9feef0a66af on yohanboniface:reduce-boilerplate into 1eed47141a75a7780fe51b3f870682c07adc14ae on Leaflet:master.

[justinmanley]

@yohanboniface - Thanks for the PR! This is awesome!

I've had the same concerns before - I completely agree that L.ToolbarAction is a lot of heavy machinery for adding simple interactions.

I'd love to see this feature included, but I'd prefer to take a slightly different approach. I'd rather see a shortcut method added to L.ToolbarAction. For example:

L.ToolbarAction.simple = function(options) {
    var SimpleAction = L.ToolbarAction.extend({
        addHooks: function() {
            if (this.options.callback) {
                this.options.callback();
            }
        }
    });

    return new SimpleAction(options);
};

This would turn your example above into:

new L.Toolbar({ actions: [
   L.ToolbarAction.simple({
        tooltip: 'Back to center',
        className: 'custom-class',
        onEnable: myCallback
    }),
    L.ToolbarAction.simple({
        tooltip: 'Back to center2',
        className: 'custom-class2',
        onEnable: myCallback2
    })
] });

I prefer this method because

1. It is more transparent. Wrapping each of the action objects in a call to L.ToolbarAction.simple makes it clear that this is syntactic sugar for the more complex machinery of L.ToolbarAction, and it makes it absolutely clear where to look to find out how the object is treated / desugared (just grep for L.ToolbarAction.simple in the source code). As a result, I think this would make it easier for users with more complex needs to upgrade from L.ToolbarAction.simple to L.ToolbarAction.
2. It requires fewer changes to the source. The changes that are required are more local. This means it is less likely to introduce bugs.

Otherwise, the contents of the PR looks good to me! I don't have any objections to removing the toolbarIcon option wrapper. I feel like a had a reason for using that at some point, but I wasn't able to find it by looking back through the commit messages, so I say take it out!

What do you think?

[yohanboniface]

Sorry for the lag, I've been pulled away. :(

I don't really share your thoughts about the .simple alternative. The simpler, the better. An object is enough to express what we need, I don't see any good reason to have a method that just takes this object as parameter. If we would go this way, Leaflet API would look like: L.Map(L.DomUtil.get('#map'), {center: L.latLng([1, 2])}) and so on, but in fact we can do L.Map('map', {center: [1, 2]}) because the API try to keep simple things simple, with a welcoming default signature. ;) I suggest we should follow this mantra. :)

[mourner]

@yohanboniface @justinmanley hey guys, any further thoughts here?