views:

320

answers:

3

Hi guys,

I think I've been too much time looking at this function and just got stuck trying to figure out the nice clean way to do it.

It's a jQuery function that adds a click event to any div that has a click CSS class. When that div.click is clicked it redirects the user to the first link found in it.

function clickabledivs() {
    $('.click').each(
        function (intIndex) {
            $(this).bind("click", function(){
                window.location = $( "#"+$(this).attr('id')+" a:first-child" ).attr('href');
            });
        }
    );
}

The code simply works although I'm pretty sure there is a fairly better way to accomplish it, specially the selector I am using: $( "#"+$(this).attr('id')+" a:first-child" ). Everything looks long and slow. Any ideas?

Please let me know if you need more details. Thanks!

PS: I've found some really nice jQuery benchmarking reference from Project2k.de here: http://blog.projekt2k.de/2010/01/benchmarking-jquery-1-4/

A: 

Instead of binding all the clicks on load, why not bind them on click? Should be much more optimal.

$(document).ready(function() {
   $('.click').click(function() {
        window.location = $(this).children('a:first').attr('href');
        return false;
    });
});
Jeriko
Where do you see anything about binding the clicks on load?
Matt Ball
A: 

I would probably do something like,

$('.click').click(function(e){
  window.location.href = $(this).find(a).attr('href');
});

Might work, not tested it :)

DavidYell
+1 because you were the first not to assume it was an immediate child!
T.J. Crowder
I'm pretty sure he just means `'a'` instead of `a`.
Matt Ball
A: 

Depending on how many of these div.click elements you have, you may want to use event delegation to handle these clicks. This means using a single event handler for all divs that have the click class. Then, inside that event handler, your callback acts based on which div.click the event originated from. Like this:

$('#div-click-parent').click(function (event)
{
    var $target = $(event.target); // the element that fired the original click event
    if ($target.is('div.click'))
    {
        window.location.href = $target.find('a').attr('href');
    }
});

Fewer event handlers means better scaling - more div.click elements won't slow down your event handling.

Matt Ball
Wow, thanks for the great and quick answers. The function could definitely be optimized much more. I'll stick with the one Bears will eat you suggests. Just one event for all the div clicks. Thank you!!
XaviEsteve
Just a quick tweak, "if ($target.is('div.click')" needs an extra ) at the end. Thanks again!
XaviEsteve
Whoops, thanks! I'll fix that.
Matt Ball