current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

This is my second jQuery script: a universal tooltip for my entire web page. It took me a couple of days to research and make it work.

Now, I need help from the experts to optimize my newbie code.

This is one of the ways I can show the tooltip:

<a class="tooltip" data-tooltip="this is the text for my tooltip" href="#">My tooltip</a>

jQuery:

$('.tooltip').hover(function(){
    var text = $(this).attr('data-tooltip');

    // get coordinates (top, left) of clicked element
    var offset = $(this).offset();

    // append tooltip to body, hide it, insert text
    // center the tooltip to the clicked element and display it 3 pixels above the clicked element
    $('<p class="tooltipbox"></p>')
    .hide()
    .text(text)
    .appendTo('body')
    .css("left", offset.left - (($(".tooltipbox").outerWidth() - $(this).width()) / 2 )) // center the tooltip
    .css("top", offset.top - $(".tooltipbox").outerHeight() - 3)
    .fadeIn(200);
}, function() {
    // hover out the tooltip box
    $('.tooltipbox').remove();
});

Something simple that works ok. Any way to improve my second jQuery script?

share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

You don't really need a tooltip class on your target element. Since you already have a data-tooltip attribute, that's enough to find the relevant elements:

$("[data-tooltip]") // find all elements with a data-tooltip attribute

This will also let you use the tooltip class for the actual tooltip, which makes more sense, given the name.

You call $(this) multiple times instead of calling it once and storing it a variable, e.g.

var target = $(this);

You're also creating an element in your code, yet you're fetching it back through jQuery. That really isn't necessary, since that can go in a variable too. And you can use jQuery to your advantage a bit more.

var tooltip = $("<p></p>")
  .hide()
  .addClass("tooltipbox")
  .appendTo(document.body);

then, when positioning it, it might be nice with yet more variables, just to improve clarity a bit

var top  = offset.top - tooltip.outerHeight() - 3,
    left = offset.left - (tooltip.outerWidth() - target.width()) / 2;

tooltip.css({
  left: left,
  top:  top
});

Also, you may want to not hide the tooltip, if the user mouses over it. Doing so will require more trickery, though, so it's a bit out-of-scope for this answer. But the point is that right now, moving over the tooltip causes it to disappear, which can cause a bunch of flickering, since it's being displayed practically where the cursor already is.

Lastly, it'd be better to use a div or span for the tooltip. While not strictly necessary, the div element is a better fit, because it's semantically neutral, and span just means "a run of text". A p element means paragraph, but a tooltip is usually just a tiny snippet; not really a paragraph in and of itself. There's nothing preventing you from using a p or any other element, but that also means you might as well use a more appropriate element than p.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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