6
\$\begingroup\$

I am writing my own tooltip plugin for my portfolio website that I am currently working on. It works rather well, however, I feel that there is a lot I could improve. This is one of my bigger jQuery plugins that I have written.

(function($) { // Used as a template for addTooltip() var tooltipDefaults = { 'class': null, 'showOn': 'mouseenter', 'hideOn': 'mouseleave', 'direction': 'top', 'offset': null } // Store generated IDs to avoid conflicting IDs var tooltipIds = new Array(); // Keep track of displayed popups var displayedTooltips = new Array(); function generateUniqueId() { var id; do { id = Math.floor(Math.random()*90000) + 10000; } while ($.inArray(id, tooltipIds) !== -1); tooltipIds.push(id); return id; } function getUniqueId(id) { return parseInt(id.substr(0, 5)); } function isUniqueId(id) { return !NaN(getUniqueId(id)); } function removeUniqueId(id) { var id = getUniqueId(id); var idIndex = $.inArray(id, tooltipIds); if (idIndex !== -1) { tooltipIds.splice(idIndex); } } $.fn.displayTooltip = function() { var element = $(this); var tooltip = $('#' + element.attr('data-tooltip-id')); var config = element.data('config'); var offset = element.offset(); var left; var top; switch (config.direction) { case 'left': top = offset.top + "px"; left = offset.left - tooltip.outerWidth() - config.offset + "px"; break; case 'top': top = offset.top - element.outerHeight() - config.offset + "px"; left = offset.left + ((element.outerWidth() / 2) - (tooltip.outerWidth() / 2)) + "px"; break; case 'right': top = offset.top + "px"; left = offset.left + element.outerWidth() + config.offset + "px"; break; case 'bottom': top = offset.top + element.outerHeight() + config.offset + "px"; left = offset.left + ((element.outerWidth() / 2) - (tooltip.outerWidth() / 2)) + "px"; break; } tooltip.css({ 'position': 'absolute', 'left': left, 'top': top, 'z-index': 5000 }); if (element.isTooltipDisplayed()) { return; } tooltip.show(); displayedTooltips.push(element.attr('id')); } $.fn.hideTooltip = function() { var element = $(this); var idIndex = $.inArray(element.attr('id'), displayedTooltips); if (idIndex !== -1) { displayedTooltips.splice(idIndex); } $('#' + element.attr('data-tooltip-id')).hide(); } $.fn.addTooltip = function(content, params) { var config = $.extend(tooltipDefaults, params); return this.each(function() { var element = $(this); // If the element already has a tooltip change the content inside of it if (element.hasTooltip()) { $('#' + element.attr('data-tooltip-id')).html(content); return; } var tooltipId = (element.is('[id]') ? element.attr('id') : generateUniqueId()) + '-tooltip'; element.attr('data-tooltip-id', tooltipId); var tooltip = $('<div>', { id: tooltipId, role: 'tooltip', class: config.class }).html(content); $('body').append(tooltip); /** * If showOn and hideOn are the same events bind a toggle * listener else bind the individual listeners */ if (config.showOn === config.hideOn) { element.on(config.showOn, function() { if (!element.isTooltipDisplayed()) { element.displayTooltip(); } else { element.hideTooltip(); } }); } else { element.on(config.showOn, function() { element.displayTooltip(); }).on(config.hideOn, function() { element.hideTooltip(); }); } // Store config for other functions use element.data('config', config); // Saftey check incase the element recieved focus from the code running above element.hideTooltip(); }); } $.fn.hasTooltip = function() { return $(this).is('[data-tooltip-id]'); } $.fn.isTooltipDisplayed = function() { var element = $(this); if (!element.hasTooltip()) { return false; } return ($.inArray(element.attr('id'), displayedTooltips) === -1) ? false : true; } $.fn.removeTooltip= function() { return this.each(function() { var element = $(this); var tooltipId = element.attr('data-tooltip-id'); var config = element.data('config'); $('#' + tooltipId).remove(); if (isUniqueId(tooltpId)) { removeUniqueId(tooltipId); } element.removeAttr('data-tooltip-id'); if (config.showOn === config.hideOn) { element.off(config.showOn); } else { element.off(config.showOn); element.off(config.hideOn); } element.removeData('config'); }); } // Reposition tooltip on window resize $(window).on('resize', function() { if (displayedTooltips.length < 1) { return; } for (var i = 0; i < displayedTooltips.length; i++) { $('#' + displayedTooltips[i]).displayTooltip(); console.log(displayedTooltips); } }); }(jQuery)); 
\$\endgroup\$
5
  • \$\begingroup\$ new Array() -> [] \$\endgroup\$ Commented Feb 16, 2014 at 14:45
  • \$\begingroup\$ if (element.isTooltipDisplayed()) { return; } could probably called before all left & top calculations... \$\endgroup\$ Commented Feb 16, 2014 at 14:52
  • 2
    \$\begingroup\$ 1.this is not good jquery plugin api style conversion. your plugin should only has one namespace. 2. not use utility in jQuery(like $.inArray), the performance is too slow \$\endgroup\$ Commented Feb 17, 2014 at 1:49
  • \$\begingroup\$ check this performance table jspref \$\endgroup\$ Commented Feb 17, 2014 at 2:01
  • \$\begingroup\$ @caoglish indexOf() \$\endgroup\$ Commented Feb 17, 2014 at 8:35

1 Answer 1

2
\$\begingroup\$

From a once over:

  • There seems to be a ton of overkill with regards to id, since the user will never look at the id of a tooltip, you can simply use:

    function generateUniqueId() { var id = generateUniqueId.id = ( generateUniqueId.id || 0) + 1; tooltipIds.push(id); return id; } 

    on the whole I would review the code and simplify how id's work.

  • Namespaces; @caoglish is right, your approach is wrong. jQuery plugin style would be 1 function tooltip on which you can call then create, show, hide etc.
  • In removeUniqueId you simply use id = getUniqueId(id); ( drop the var ), since you already have the id as a parameter
  • As @Charlie mentioned; var tooltipIds = new Array(); -> var tooltipIds = [];
  • In removeTooltip you have a bug in (isUniqueId(tooltpId)) { , you meant to use tooltipId
  • You would have caught this if you added "use strict"; right after (function($) { (this activates strict mode)
  • I really like the way you use config in displayTooltip
  • console.log() does not belong in production code
\$\endgroup\$
5
  • \$\begingroup\$ Thank you a lot! I will be sure to make these changes. Can you possibly explain to me how that generateUniqueId method works? \$\endgroup\$ Commented Feb 19, 2014 at 16:47
  • \$\begingroup\$ I am using the function to store the value of id and I just keep adding 1 so that the value is unique. \$\endgroup\$ Commented Feb 19, 2014 at 16:57
  • \$\begingroup\$ Alright, also is it really ideal to only use one namespace. It just seems cluttered to toss everything into one namespace. Or is there a design pattern that could make something like that better? \$\endgroup\$ Commented Feb 19, 2014 at 16:59
  • \$\begingroup\$ I will agree that it might be more cluttered for the author, but the convenience of the user comes first ;) \$\endgroup\$ Commented Feb 19, 2014 at 17:02
  • \$\begingroup\$ I just rewrote the whole plugin to an OOP approach. I also made the changes you suggested and the whole ID system is now out. I dropped about 80 lines of code. How much better is this than the previous one and are there still areas to be improved? If you wouldn't mind taking a look that is. \$\endgroup\$ Commented Feb 19, 2014 at 18:05

You must log in to answer this question.