1
\$\begingroup\$

Here's a working version of my code - http://jsfiddle.net/sambenson/g3qsa/#base
How can this be improved upon?

$(function(){
    var box = {},
        island = {},
        canvas = {};

    box.el = $('#box');
    canvas.el = $('#arrow');

    function go(el){
        if(box.el.css('visibility') == 'hidden'){
            reset_name();
            $('.island').addClass('inactive');

            box.top = box.el.offset().top;
            box.corner = {};
            box.corner.left = box.el.offset().left;
            box.corner.right = box.corner.left + box.el.width();

            island.el = el;
            island.top = island.el.offset().top;
            island.left = island.el.offset().left;

            island.el.removeClass('inactive').addClass('active');

            canvas.width = island.left - box.corner.left;
            canvas.height = island.top - box.top;

            canvas.el.css({
                top: box.top,
                left: box.corner.left
            }).attr({
                width: canvas.width,
                height: canvas.height    
            });

            var context = canvas.el[0].getContext('2d');

            context.beginPath();
            context.moveTo(0, 0);
            context.lineTo(box.el.width(), 0);
            context.lineTo(canvas.width, canvas.height);
            context.closePath();

            context.fillStyle = "#eaeaea";
            context.fill();

            box.el.css('visibility', 'visible');
            canvas.el.show();
        }
        else {
            close();
        }
    }

    function reset_name(){
        $('#name_holder').text('').css({
            top: -9999,
            left: -9999
        })
    }

    function close(){
        $('.island').removeClass('inactive active');
        box.el.css('visibility', 'hidden');
        canvas.el[0].width = canvas.el[0].width;
        canvas.el.hide();
    }

    $('.island').hover(function(){
        if(box.el.css('visibility') == 'hidden'){
            var t = $(this),
                name = t.text(),
                holder = $('#name_holder');
                top = t.offset().top - 60;
                left = t.offset().left - 50;

            holder.css({
                top: top,
                left: left
            }).text(name);
        }
    }, function(){ reset_name(); })
    .click(function(){ go($(this)); })

    $('.close').click(function(){
       close();
    })


})

Could it be made faster?
Could it be made more efficient?

\$\endgroup\$
0
2
\$\begingroup\$

Along with @kingjv's suggestions, you can improve the performance of your jQuery selections by making some simply tweaks to the selectors.

Consider if I have the following html:

<div id="container">
    <div class="cell">Cell 1</div>
    <div class="cell">Cell 2</div>
</div>

If you I am using a class selector, such as $(".cell") then the selector engine has to do a little more than it needs to. In this example, the selector engine has to walk the entire DOM to match the correct elements. We can improve this a couple of ways:

  • Specify a tag in the selector, e.g. $("div.cell"). This allows the selector engine to use the native getElementsByTagName method to shrink the number of comparisons the engine has to do to match.

  • Specify a root object to start searching from. E.g., if I've previously cache my selector var root = $("#container"), I can reuse that selector: $("div.cell", root) that way we don't walk the entire DOM for matches.

These are small performance improvements, you'll appreciate them more if you are doing a lot of DOM manipulation and lookup. More simplistic applications won't have such an apparent performance improvement.

\$\endgroup\$
2
\$\begingroup\$

My comments are more just on style as opposed to performance:

Instead of using :

if(box.el.css('visibility') == 'hidden'){

it's more readable to use :

if(box.el.is(":hidden")){

And similarly, instead of using :

box.el.css('visibility', 'visible');

you can use :

box.el.hide();

A note on this one though, these are not exactly equivalent because hide actually sets display: none. In this case though I believe that's fine.

And one final comment, it's a good practice to name variables which contain jQuery objects with a $ at the beginning. This allows you to know that the variable is already a jQuery object without going back in the code to check and avoids accidentally creating jQuery objects out of objects that were already jQuery objects.

So something like this:

var t = $(this),
    name = t.text()

would become this:

var $t = $(this),
    name = $t.text()
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your review. With regards to your first point: I had to use visibility: hidden instead of .hide() etc because the jQuery needs to be able to measure the width of the box whilst it's hidden. Your $variable_name suggestion is great though! \$\endgroup\$
    – Sam
    Jul 14 '11 at 14:05
  • \$\begingroup\$ Ah okay, I looked to see if you had visibility for a reason but missed that. Can still use :hidden though I believe. \$\endgroup\$ Jul 14 '11 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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