Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

All tooltips rendered on hover - one is enough #85

Open
arski opened this Issue Jul 13, 2012 · 4 comments

Comments

Projects
None yet
2 participants

arski commented Jul 13, 2012

For any chart, when you hover over one data point, a tooltip is actually generated for every data point in the chart - the other ones aren't shown, but this results in potentially hundreds of div elements being added, moved, etc.. resulting in really poor performance, not to mention that this is completely useless.

To fix, in Rickshaw.Graph.HoverDetail render() function, move if (d.active) { from covering the last two lines of the loop, to the very top of the loop, just before var item = document.createElement('div'); - that was only one element is created and shown.. all that needs.

I'm sure this can be made even more efficient, but this would be a good start.

arski commented Jul 13, 2012

Actually, you can also stop the loop once you've found the correct series that needs to have the tooltip shown. Something like the following should do:

    detail.sort(sortFn).forEach( function(d) {

        if (domainMouseY > d.value.y0 && domainMouseY < d.value.y0 + d.value.y && !activeItem) {
            d.formattedYValue = (this.yFormatter.constructor == Array) ?
                this.yFormatter[detail.indexOf(d)](d.value.y) :
                this.yFormatter(d.value.y);

            d.graphX = graphX;
            d.graphY = graph.y(d.value.y0 + d.value.y);

            activeItem = d;
            return;
        }

    }, this );

arski commented Jul 13, 2012

oh and then, you can do

    if (this.visible && activeItem) {
        this.render( {
            activeItem: activeItem,
            domainX: domainX,
            formattedXValue: formattedXValue,
            mouseX: eventX,
            mouseY: eventY
        } );
    }

and the render function then doesnt need any loops

render: function(args) {

    var activeItem = args.activeItem;
    var domainX = args.domainX;

    var mouseX = args.mouseX;
    var mouseY = args.mouseY;

    var formattedXValue = args.formattedXValue;

    var xLabel = document.createElement('div');
    xLabel.className = 'x_label';
    xLabel.innerHTML = formattedXValue;
    this.element.appendChild(xLabel);

    var item = document.createElement('div');
    item.className = 'item';
    item.innerHTML = this.formatter(activeItem.series, domainX, activeItem.value.y, formattedXValue, activeItem.formattedYValue, activeItem);
    item.style.top = this.graph.y(activeItem.value.y0 + activeItem.value.y) + 'px';

    this.element.appendChild(item);

    var dot = document.createElement('div');
    dot.className = 'dot';
    dot.style.top = item.style.top;
    dot.style.borderColor = activeItem.series.color;

    this.element.appendChild(dot);

    item.className = 'item active';
    dot.className = 'dot active';

    this.show();

    if (typeof this.onRender == 'function') {
        this.onRender(args);
    }
},

arski commented Jul 13, 2012

and my apologies, I'm still not too familiar with github in terms of providing patches.. its kinda complicated when i have quite a few more changes to the file too that are unrelated.. hope this works.

Contributor

dchester commented Jul 18, 2012

All good points.

Things evolved as they did because we started with a bunch of use cases where it was actually useful to show details for all of the series for a given time at once. Then we hid "inactive" tooltips by default, but the consumer can still show them by overriding some CSS.

But I think that's mostly an edge case. We'll pursue going forward with the optimizations you suggested, and maybe just provide an example subclass that shows all the data for a given time at once if that's what to do.

and my apologies, I'm still not too familiar with github in terms of providing patches.. its kinda complicated when i have quite a few more changes to the file too that are unrelated..

Pull requests are pretty awesome: https://help.github.com/articles/using-pull-requests/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment