2
\$\begingroup\$

I have the following piece of code:

for (var i = 0, len = legend.allItems.length; i < len; i++) {
    (function (i) {
        var item = legend.allItems[i].legendItem;

        item.on('mouseover', function (e) {
            var posLeft = 0,
            posTop = 0, 
            parentWidth = chart.chartWidth;

            var html = '<span class="legend-tooltip">' + 
                MetricsTooltips.get(chart.series[i].options.tooltipKey) + 
            '</span>';

            labelElement = chart.renderer.label(html, posLeft, posTop, 'callout', null, null, true, false, 'legend-tooltip').attr({zIndex: 10}).add();

            if(legend.group.alignOptions.align == 'center') {
                // _legendItemPos[0] returns the offset (left) for current legend item
                posLeft = legend.group.translateX + legend.allItems[i]._legendItemPos[0];
                // Subtract some "offset" as we don't want tooltips to overlap the legend
                posTop = legend.group.translateY - 30;

                // Make sure that tooltip won't overflow its parent
                if ((legend.allItems[i]._legendItemPos[0] + labelElement.width) > parentWidth) {
                    posLeft = parentWidth - labelElement.width;
                }

                labelElement.destroy();
            } else if (legend.group.alignOptions.align == 'right') {
                posTop = (legend.group.translateY + legend.allItems[i]._legendItemPos[1]);
                posLeft = (parentWidth - legend.group.alignOptions.width) - labelElement.width;

                if(legend.scrollGroup.translateY && legend.currentPage > 1) {
                    // Handle the case when legend group have too many metrics and paging appears in
                    posTop -= Math.abs(legend.scrollGroup.translateY);
                }

                labelElement.destroy();
            } else {
                // handle this if needed
            }

            labelElement = chart.renderer.label(html, posLeft, posTop, 'callout', null, null, true, false, 'legend-tooltip').css({zIndex: 10}).add();

        }).on('mouseout', function (e) {
            labelElement.destroy();
        });
    })(i);
}

Basically, it attach event listeners on all legend items when chart is loaded (the for is located inside onload callback), and later, when mouseenter event occurs, we display simple "tooltip" to the user.

It's working very well, but I'm looking for some refactoring hints.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

My observations thus far:

  • You are not actually using mouseenter in your code, you should as that would be the better approach
  • The function inside a loop with the double var assignment is awkward, if you want to go functional just use .forEach()
  • The way you have // handle this if needed, the reader is not sure if that can happen. If it can happen, then the code will create 2 times labelElement since you did not call labelElement.destroy(); in that else branch. If it cannot happen, remove the code. Also if it cannot happen, place labelElement.destroy(); after the if statement
  • labelElement seems to be a global, I hope it is created with a var statement elsewhere in your code
  • From a naming perspective (not sure this is your naming), I have no idea what scrollGroup would return, and even less of an idea now that I see that that object has a method called translateY. You might want to re-consider carefully some of the naming.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.