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.

So after answering a question about nested linked checkboxes I mentioned it at work and surprisingly it turned out to be similar to something we needed.

The requirements were:

  1. a "Select all" checkbox
  2. a nested "Select all" checkbox (only selects a small portion)
  3. Cannot rely upon the hierarchy of the markup. The given fiddle is an example only. EDIT:- from Discussions with John I now see there was this requirement and so I've added it.

So realising it was different to the aforementioned question I set about writing my own. I aimed for readability and minimal markup.

(function($) {
    $('input[type=checkbox][data-children]').unbind("change childchange childchangebubble").each(function() {
        var parent = $(this);

        var linkedChildren = $(parent.data("children"));

        //trigger a "childChange" event on the parent when any child is triggered.
        linkedChildren.bind("change childchangebubble", function() {
            parent.trigger("childchange");
        });

        parent.bind({
            "change": function(event) { // Bind change event to check all children
                linkedChildren.prop("checked", parent.prop("checked")).trigger("change");
            },
            "childchange": function() { // have to bind custom event seperately, there seems to be a jQuery bug.
                // When a child is changed recalculate if parent should be checked
                var noChildrenUnchecked = !linkedChildren.is(":not(:checked)");
                parent.prop("checked", noChildrenUnchecked).trigger("childchangebubble");
            }
        });
    });
})(jQuery);

the data-children attributes that are rendered to the page look like: data-children="#child2sub1, #child2sub2" on the parent and nothing extra needed on the children.

I'd love any feedback on readability/maintainability, performance etc.

current jsFiddle.

share|improve this question
    
Adding a bounty on this. If you have even a small improvement for me please post it! –  James Khoury Sep 26 '11 at 4:52

2 Answers 2

up vote 1 down vote accepted
+50

Some small changes:

  1. Change input[type=checkbox] to :checkbox. Won’t do much except make things look cleaner.
  2. It looks like you are using the data-children value as jQuery selectors and using all the id’s. if you want to shorten this you could add a specific css class on them and put that value as the data-children. Your code would still work and the markup would probably be shorter.
  3. Also you should be careful as your code unbinds “change” which could unbind events from other jQuery code and this would probably break it.
share|improve this answer

I have a suggestion. Basically I'm not sure why you are adding so much to the HTML to handle the the parent and child checkboxes. Isn't the hierarchy of the HTML enough to go on? From the HTML you can already determine the parent and child relationships. In additon why trigger more events. Just handle the event as it bubbles up though the DOM.

Below is an example of some code that I believe to be far more reusable and much easier to implement. In addition, I have modified this to work with future checkboxes added to the DOM. It may be necessary to load some of the children via AJAX and/or appending to the DOM therefore your method would require you to bind your function on each append to the DOM, where as mine would handle it though delegation of events.

$.fn.nestedCheckboxes = function (stopPropogation) {
    this.addClass('nestedCheckboxes');
    this.click(nestedCheckboxHandler);
    this.delegate(':has( > input:checkbox)', 'click', nestedCheckboxHandler);
    function nestedCheckboxHandler (evt) {
        if ($(evt.target).is("input:checkbox")) {
            var parentContainer = $(this);
            var checkbox = parentContainer
                            .find(' > input:checkbox:eq(0)');
            var parentCheckbox = parentContainer
                                    .parent()
                                    .closest(':has( > input:checkbox)')
                                    .children('input:checkbox');

            if (evt.target == checkbox[0]) {
                $('input:checkbox', parentContainer)
                    .prop("checked", checkbox.prop("checked"));
            }
            var parentCheckboxValue = !parentCheckbox
                                        .parent()
                                        .find("input:checkbox")
                                        .not(parentCheckbox[0])
                                        .is("input:checkbox:not(:checked)");

            parentCheckbox.prop("checked",parentCheckboxValue);
        } else if (stopPropogation) {
            evt.stopPropagation();
        }
    };
};

$(function () {
    $('div').nestedCheckboxes();  //this is how you would use the function

    $('#addChildren').click(function () {
       $(this).after('<ul><li><input type="checkbox" /><label>Child 2 SubChild 1</label></li><li><input type="checkbox" /><label>Child 2 SubChild2</label></li></ul>');
       return false;
    });    

});

In addition I have added an option for the nestedCheckboxes to stopPropogation if an element other than a checkbox is clicked. By default this if set to false.

Finally have a look at the fiddle I created and play around. You will see that the HTML Markup is far more simplistic.

http://jsfiddle.net/jfhartsock/WwtJr/

share|improve this answer
    
" Isn't the hierarchy of the HTML enough to go on?" Simply: No. The checkboxes are spread across the html as it had to be added to an old form. Some of the checkboxes are in sequential rows of a table and some are not. So the order of the html cannot be relied upon. (and imho I don't think it should.) –  James Khoury Sep 29 '11 at 0:22
    
One small quibble I would add is the "add two children button doesn't update/refresh the checkboxes after they add so their parent checkbox might be check when they get added and they won't. In that situation either the parent needs to be unchecked or the children need to be checked. In my code i just added a .trigger("childChange") at the end. –  James Khoury Sep 29 '11 at 0:32
    
But your binding event would never bind you would have to handle that manually as it does not handle future events. You should consider handling events using delegate() instead. But in any case it was just a thought as the html in your example showed you following the hierarchy in html. It my be wise to put "Some of the checkboxes are in sequential rows of a table and some are not." as a specific requirement. –  John Hartsock Sep 29 '11 at 0:35
    
in the spec I should have put the restriction that the layout f the markup cannot be relied on I just assumed it couldn't unless other specified. the .trigger("childChange") works because the function is run it will unbind the previously bound events. It could be modified to only bind on the ones it needs but I didn't see it as a need. Can you? –  James Khoury Sep 29 '11 at 0:41
    
No I guess not. Just offering a different point of view. –  John Hartsock Sep 29 '11 at 0:50

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.