15
\$\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\$
5
  • \$\begingroup\$ @Jonathan, ha, nothing really. It just seems that everyone these days starts 'programming' with html, css, and JQuery. \$\endgroup\$
    – jjnguy
    Feb 18, 2011 at 13:34
  • \$\begingroup\$ @zzz Yeah, I know, thanks. I have fixed it in my code. \$\endgroup\$
    – jjnguy
    Feb 19, 2011 at 16:42
  • \$\begingroup\$ You should run your first jQuery in the Firebug Console. \$\endgroup\$
    – powtac
    Feb 20, 2011 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, 2011 at 1:56
  • \$\begingroup\$ @alpha, haha that's good to know. \$\endgroup\$
    – jjnguy
    Aug 15, 2011 at 2:04

2 Answers 2

15
\$\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\$
7
  • \$\begingroup\$ Why do you declare expandAll and colapseAll outside of the doc.ready() function? \$\endgroup\$
    – jjnguy
    Feb 17, 2011 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\$ Feb 17, 2011 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, 2011 at 12:36
  • \$\begingroup\$ OCD alert: collapse has two Ls (yes, I know the OP has the same typo). \$\endgroup\$
    – Matt Ball
    Feb 18, 2011 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, 2011 at 18:34
3
\$\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\$
4
  • \$\begingroup\$ One question, why do you use html and text in different scenarios? \$\endgroup\$
    – jjnguy
    Feb 23, 2011 at 17:07
  • \$\begingroup\$ $this.nextAll('.cont:first').toggleClass('hid'); was what worked. \$\endgroup\$
    – jjnguy
    Feb 23, 2011 at 17:07
  • \$\begingroup\$ @Justin 1. It was a mistake. Fixed. 2. I don't get what are you trying to say… \$\endgroup\$
    – Alexia
    Feb 23, 2011 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, 2011 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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