13
\$\begingroup\$

The general idea of the code is that I have a + or - icon (".trigger") that is click-able and will expand or collapse the content (".cont") directly following it (there are many of there expand/collapse pairs). I also have a span("#expandAll") that, when clicked, will expand or collapse every block.

Please review my first usage of jQuery and help me put my head into a more jQuery state. Where can the code be improved? Are there any problems with it? Is there a better way to do it?

(To be perfectly honest, I have no idea what the $() function really does.)

var expandAll = function () {
    $(".cont").removeClass("hid");
    $("#expandAll").html("Colapse All");
    $("#expandAll").unbind("click", expandAll);
    $(".trigger").html("-");
    $("#expandAll").click(colapseAll);
}
var colapseAll = function () {
    $(".cont").addClass("hid");
    $("#expandAll").html("Expand All");
    $("#expandAll").unbind("click", colapseAll);
    $(".trigger").html("+");
    $("#expandAll").click(expandAll);
}
$(document).ready(function () {
    $(".trigger").click(function (event) {
        $this = $(this);
        $this.html($this.text() == '+' ? '-' : '+');
        $($this.nextAll(".cont").get(0)).toggleClass("hid");
    });
    $("#expandAll").click(expandAll);
});

Please let me know if you need more/less info.

\$\endgroup\$
  • \$\begingroup\$ @Jonathan, ha, nothing really. It just seems that everyone these days starts 'programming' with html, css, and JQuery. \$\endgroup\$ – jjnguy Feb 18 '11 at 13:34
  • \$\begingroup\$ @zzz Yeah, I know, thanks. I have fixed it in my code. \$\endgroup\$ – jjnguy Feb 19 '11 at 16:42
  • \$\begingroup\$ You should run your first jQuery in the Firebug Console. \$\endgroup\$ – powtac Feb 20 '11 at 21:13
  • \$\begingroup\$ "To be perfectly honest, I have no idea what the $() function really does." Don't worry, nobody knows everything it does. I've heard it can create world peace. Hey, John Resig wrote it, right? \$\endgroup\$ – Peter C Aug 15 '11 at 1:56
  • \$\begingroup\$ @alpha, haha that's good to know. \$\endgroup\$ – jjnguy Aug 15 '11 at 2:04
14
\$\begingroup\$

I've refactored your code and added comments to explain certain things.

// We can shorten this from document.ready(...) to $(...) 
// Internally, jQuery will add the passed in function to a list
// of handlers that will be invoked when the document is ready.
$(function() {
    // Since the $ function will query the document with the 
    // specified selector we want to cache these.
    // Doing this from within the ready callback allows us 
    // to keep these references hidden.
    // I've also added the 2 function variables to keep them
    // hidden as well.
    var expandAll, colapseAll,
        expandAllElement = $("#expandAll"),
        contElements = $(".cont"),
        triggerElements = $(".trigger");

    expandAll = function() {
        contElements.removeClass("hid");
        triggerElements.html("-");
        // Notice how we can chain these calls together?
        // jQuery was designed with a fluent interface.
        expandAllElement.html("Colapse All").one("click", colapseAll);
    };

    colapseAll = function() {
        contElements.addClass("hid");
        triggerElements.html("+");
        expandAllElement.html("Expand All").one("click", expandAll);
    };

    triggerElements.click(function(event) {
        // Always define a variable so the 'name' 
        // is not defined in the global object,
        var $this = $(this);
        $this.html($this.text() == '+' ? '-' : '+');
        $($this.nextAll(".cont").get(0)).toggleClass("hid");
    });

    // Using the one function allows you to skip 
    // unbinding the event handler for each click.
    expandAllElement.one("click", expandAll);
});
\$\endgroup\$
  • \$\begingroup\$ Why do you declare expandAll and colapseAll outside of the doc.ready() function? \$\endgroup\$ – jjnguy Feb 17 '11 at 22:22
  • \$\begingroup\$ @Justin - You know I wasn't sure if you were manually calling those functions from other places. If your not using the functions anywhere else then that is definitely a good idea. See my latest changes. \$\endgroup\$ – ChaosPandion Feb 17 '11 at 22:37
  • 1
    \$\begingroup\$ Hm there's no need for making expandAll and colapseAll expressions, declarations would do just fine here. Also both functions are extremely similar in their workings, refactoring the code out into a more generic helper would be a good idea here. \$\endgroup\$ – Ivo Wetzel Feb 18 '11 at 12:36
  • \$\begingroup\$ OCD alert: collapse has two Ls (yes, I know the OP has the same typo). \$\endgroup\$ – Matt Ball Feb 18 '11 at 17:01
  • \$\begingroup\$ @Matt, yeah I didn't notice that spelling fail until after I posted the code. Didn't feel like fixing it. \$\endgroup\$ – jjnguy Feb 18 '11 at 18:34
2
\$\begingroup\$
// If you use selectors multiple times, always cache them into variables, else jQuery has to search for them multiple times.
var $cont      = $('.cont'),
    $expandAll = $('#expandAll'),
    $trigger   = $('.trigger');

function expandAll() {
    $cont.removeClass('hid');
    $trigger.text('-');
    // Use .text() instead of .html().
    // You can chain these calls together.
    // If you use .one(), you don't have to unbind the previous event handler next time, jQuery does it for you.
    $expandAll.text('Collapse All').one('click', collapseAll);
}
function collapseAll() {
    $cont.addClass('hid');
    $trigger.text('-');
    $expandAll.text('Expand All').one('click', expandAll);
}

// You can pass a function to jQuery itself – it's a shorthand for $(document).ready().
$(function() {
    // There's no need to pass the event argument, because
    // 1. You didn't use it there
    // 2. jQuery “normalizes” the event object, so you don't have to pass it as an argument. If you want a shorthand for it, do it inside the function:
    // var e = event;
    $trigger.click(function() {
        // Always use var to declare variables, to prevent polluting the global scope.
        var $this = $(this);
        // Again: use .text() instead of .html().
        $this.text($this.text() == '+' ? '-' : '+');
        // There's no need to do this:
        // $($this.nextAll(".cont").get(0)).toggleClass("hid");
        // You can simply do this:
        $this.nextAll('.cont').toggleClass('hid');
    });
    $expandAll.one('click', expandAll);
});
\$\endgroup\$
  • \$\begingroup\$ One question, why do you use html and text in different scenarios? \$\endgroup\$ – jjnguy Feb 23 '11 at 17:07
  • \$\begingroup\$ $this.nextAll('.cont:first').toggleClass('hid'); was what worked. \$\endgroup\$ – jjnguy Feb 23 '11 at 17:07
  • \$\begingroup\$ @Justin 1. It was a mistake. Fixed. 2. I don't get what are you trying to say… \$\endgroup\$ – nyuszika7h Feb 23 '11 at 17:10
  • \$\begingroup\$ the way my dom was nextAll('.cont') didn't work. I needed to add the :first. Not really important info for you. \$\endgroup\$ – jjnguy Feb 23 '11 at 18:25

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.