Added "onHover" functionality for legend

[d4popov]

• core.legend.js
  • Created an "onHover" property in the legend object that will hold a user defined function (default is null)
  • Modified the "handleEvent" function of the legend to check if the event passed to it is a "click/mouseup" or a "mousemove" to determine whether to call the "onClick" or the "onHover" event (this is for the sake of reusing the legend hitbox calculations)
  • The "onHover" event will receive the same parameters as the "onClick" (the "event" itself and the "legendItem" the user is hovering over)
• core.controller.js
  • Added a "mousemove" event to the "eventHandler" of the controller and put the new legend "onHover" logic in it
  • Also moved the original chart "onHover" functionality under the "mousemove" clause too in order to put the hover events in one place for consistency.

[simonbrunel]

Why are we testing me.legend.options.onHover?

[d4popov]

Because if the user hasn't initialized the function onHover in their chart cofig, we won't need to go through the whole process of calculating the mouse position and if it is within the boundaries of the legend hitboxes. For the onClick event, it doesnt matter so much because it's activated once, but on the onHover it will be executed continuously so with that check I am not allowing the code to propagate to calling the legend handleEvent function. If I allow the code to propagate to the handleEvent function it will do all those mouse coordinate calculations, go through the forloop checking each and every legend hitbox and if the mouse is in it to eventually find that onHover is null and not do anything.

[simonbrunel]

I'm even thinking that this eventHandler should simply forward the event to the legend handleEvent and so delegate all the event logic to the legend. Meaning, no e.type check here, simply:

if (me.legend && me.legend.handleEvent) {
    me.legend.handleEvent(e);
}

Which also mean that the legend handleEvent should discard useless events asap:

handleEvent: function(e) {
    var me = this;
    var type = e.type;

    type = type === 'mouseup'? 'click : type;
    if (type === 'mousemove') {
        if (!options.onHover) {
            return;
        }
    } else if (type === 'click') {
        if (!options.onClick) {
            return;
        }
    } else {
        return;
    }

    var position = ...
}

[simonbrunel]

Agree, the extra calculations should not be done if useless, but this is the responsibility of the legend, not the controller (since this logic is defined by the legend). I updated my previous comment accordingly.

[d4popov]

I see. Thanks for pointing that out Simon. I'll update my code and submit again.

[simonbrunel]

LGTM, Travis is failing because there are some space indents. After fixing it, can you squash your commits? I like your commit messages, so feel free to describe the same way your squashed commit :)

[d4popov]

I did fix the spacing. By the way, what do you mean by squash my commits? I haven't done it before. I am using Source Tree by the way.

EDIT: I think I did something... Not sure if I did the correct thing :/

[simonbrunel]

I mean, combine all your commits into a single one. After squashing commits, don't forget to force push to your branch (push -f). Then you should only see one commit in the commits tab.

Your last commit is not correct because you compute position before testing if it needs to (it should come after checking event type and callback presence). However you right, opts must be initialized before the checks.

[simonbrunel]

And you will need to update some tests because there is a new options: onHover. Try gulp unittest to see where it's failing.

[etimberg]

I am good to merge this once passing

[d4popov]

I hope that fixed it. Sorry for the commits have turned out to be such a jungle... I have never before done commit squashing. If there is anything else to fix, please do tell me and i'll try my best.

[simonbrunel]

I think you squashed them correctly but forgot to force push to your branch. Also, I'm not sure why there are these merge commits. But no worries, I will squash them for you when merging :)

Anyway, I realize that I made a small typo in the code example in my previous comment, can you please change:

        var type = e.type;
        type = type === 'mouseup' ? 'click' : type;

by

        var type = e.type === 'mouseup' ? 'click' : e.type;

[d4popov]

I was actually thinking about it, but then left it alone because I thought you wanted it like that. I'll fix it right away and push again.

[simonbrunel]

Yeah, that was just example code, I don't mind if it's not exactly the same if it can be better :) I think I edited my comment to merge the mouseup and click events, but initially just defined var type = e.type. Anyway, thanks for your contribution, will merge it really soon.

[d4popov]

Happy to hear that :) This is my first contribution to anything, so I wasn't very sure what was going on. I guess I need to read up some tutorials on indepth repository management and stuff.

[simonbrunel]

Forgot one important thing: can you update the document to add the new onHover callback?

[d4popov]

I added the description of the new function to the doc file like you suggestes, and took the liberty of doing a small change to the "onClick" description as well. I copied the "onClick" description for the "onHover" function and wasn't sure if I should use the words "mouse hover" or "mousemove". So I ended up using the original event naming "...'mousemove' event is registered...", and then did the same for "onClick". Previously it was "...click is registered..." and changed it to "...'click' event is registered...".

I hope thats all right.

[simonbrunel]

I think it's fine, if it's not, we will fix it later when cleaning the doc.