2
\$\begingroup\$

I'm getting started with my first official jQuery plugin, and I'm just turning now looking for input. I want to know what I'm doing wrong or right so far and if everything is being developed in a manner that won't cause issue down the road. With no further adieu:

$.fn.heftyBox = function(args) {
    if ($(this).length) {
    //Set up defaults
    var a    =  $.extend({
        type   :  "checkbox",
        width  :  "auto",
        height :  ($(this).innerHeight() > 150) ? $(this).innerHeight() : 150
    }, args);

    //Gather original attributes, convert DOM, then reassgign attributes
    var attributes = $(this)[0].attributes;
    var optionsHTML = $(this).html();
    $(this).after('<ul id="tmpul">' + optionsHTML + '</ul>');
    $(this).remove();
    var ul = $('#tmpul')[0];
    for (var i = 0; i < attributes.length; i++) {
        $(ul).attr(attributes[i].name, attributes[i].value);
    }


    //Convert options to checkbox or radios
    var options = $(ul).children('option');
    var name = $(ul).attr('name');
    $(ul).removeAttr('name');
    var f = 0;
    $.each(options, function(key, option) {
        var itemAttributes = $(this)[0].attributes;
        var value = $(this).attr('value');
        var label = $(this).text();
        var selected = $(this).attr('selected') ? "checked" : ''
        selected += $(this).attr('disabled') ? " disabled" : ''
        var newLi;
        $(this).replaceWith(newLi = $('<li ' + selected + '><input type="' + a.type + '" id="option_' + name + '_' + f + '" name="' + name + '" value="' + value + '" ' + selected + '/><label for="option_' + name + '_' + f + '">' + label + '</label></li>') )
        for (var i = 0; i < itemAttributes.length; i++) {
            $(newLi).children('input').attr(itemAttributes[i].name, itemAttributes[i].value);
        }
        f++;
    })

    //Add Filter Box
    $(ul).before('<input id="' + name + '_filter" class="list_filter" />');
    var list = $(ul).children('li');
    var filter = $('#' + name + '_filter');
    filterBox($(filter), list);

    //Contain it all
    $(filter).before('<div class="heftyBox" id="' + name + '_container"></div>');
    var container = $('#' + name + '_container');
    $(filter).appendTo($(container));
    $(ul).appendTo($(container));

    //Select all box for checkboxes
    if (a.type == "checkbox") {
        $(filter).after($('<a href="#">Select All</a>').bind('click', function() {
            var checkboxes = $(this).next('ul').find('input:not([disabled])');
            if ($(checkboxes).length > $(checkboxes + ':checked').length) {
                $(checkboxes).attr('checked', 'checked').closest('li').attr('checked', 'checked');
            } else {
                $(checkboxes).removeAttr('checked', 'checked').closest('li').removeAttr('checked');
            }
            ($(this).text() == "Select All") ? $(this).text("Select None") : $(this).text("Select All")
            $(this).next('ul').trigger('change')
            return false;
        }))
    }

    //Write the Data to the DOM
    $(ul).data({
        heftyBox: a
    })

    //Apply DOM data
    updateheftyBox($(ul));

    //Handle Value Change
    $(this).bind('change', function() {updateheftyBox($(this));})
    }

    //scroll to first selected DOM
    if ($(ul).find('li[checked]:first').length) {
        var itemTop = $(ul).find('li[checked]:first').offset().top || $(ul).offset().top;
        var ulTop = $(ul).offset().top;
        $(ul).scrollTop(itemTop - ulTop);
    }
}

updateheftyBox = function(target) {
    var a = $(target).data().heftyBox;
    var container = $(target).parent('.heftyBox');
    var filter = $(target).siblings('.list_filter');
    var ul = $(target);

    //Gather created data
    a.value = [];
    $(ul).find('input:checked').each(function() {
        a.value.push($(this).val())
    })

    $(container).css({
        width: a.width,
        height: a.height,
        "min-width": $(filter).outerWidth()
    });
    $(ul).css({
        height: a.height - $(filter).outerHeight(),
        "margin-top": $(filter).outerHeight()
    })
    $(ul).val(a.value);
}


filterBox = function(target, blocks) {
    $(target).unbind('keyup change');
    $(target).bind('keyup change', function() {
        var inText = $(this).val().trim(); //remove trailing whitespace

        $.each(blocks, function() {
            var title = $(this).children('label').text(); //the title in the block
            if (matchAll(title, inText)) {
                $(this).show();
            } else {
                $(this).hide();
            }
        })

    })
}

matchAll = function(string, args) { //string= string to match, args= search input
    var die = 0; //return switch
    var checks = args.split(' '); //break input into array
    $.each(checks, function() {
        var myReg = new RegExp(this, 'i'); //search term to regex
        if (!string.match(myReg)) { //if it doesn't match, kill the function
            die = 1;
        } 
    })

    if (die == 1) {
        return false;
    } else {
        return true;
    }
}

$('.heftyBox li:has(input:checkbox)').live('click', function() {
    ($(this).has(':checked').length) ? $(this).attr('checked', 'checked') : $(this).removeAttr('checked')
})

The concepts of thus plugin:

  • The HeftyBox should be dynamic and not affect any attributes or user input that coould be collected by the original Select box
  • The HeftyBox should accept many options to match styling and uniqueness of the application it is being used for
  • The HeftyBox should be able to be modified and customized with calls to the object after it is initiated
  • The HefyBox should be easy to implement, attractive, and functional with defualt setting

Source github/kmacey1249/heftybox

Implemented like so:

Initial HTML

<select name="test" id="test" multiple="multiple" bar="baz">
  <option value="1">One</option>
  <option value="2">Two</option>
  <option value="3" foo="bar">Three</option>
</select>

jQuery Call

$('#test').heftyBox(); //{type: "checkbox"/"radio", width: "auto", height: 150}

Result:

<div class="heftyBox" id="test_container" style="width: auto; height: 178px; min-width: 155px; ">
  <input id="test_filter" class="list_filter" />
  <a href="#">Select All</a>
  <ul id="test" multiple="multiple" bar="baz" style="height: 155px; margin-top: 23px; ">
    <li>
      <input type="checkbox" id="option_test_0" name="test" value="1">
      <label for="option_test_0">One</label>
    </li>
    <li>
      <input type="checkbox" id="option_test_1" name="test" value="2">
      <label for="option_test_1">Two</label>
    </li>
    <li>
      <input type="checkbox" id="option_test_2" name="test" value="3" foo="bar">
      <label for="option_test_2">Three</label>
    </li>           
  </ul>
</div>

plugin preview

Code in action at JSFiddle http://jsfiddle.net/kmacey1249/vXGke/

\$\endgroup\$
  • \$\begingroup\$ Can you give us an use case, some HTML which one it will work. It'll be more easy to see how it works and what it is doing. I already can see some optimization but with an use case, it will help us to review. \$\endgroup\$ – dievardump Feb 6 '12 at 20:06
  • \$\begingroup\$ Updated with example \$\endgroup\$ – Kyle Macey Feb 6 '12 at 20:58
2
\$\begingroup\$

See comments in the code that explain why I did some things.

Here is the result of my refactoring:

  • Putting the code in a self-executing function. Your three helpers functions don't have to be in the global scope.
  • Caching almost all elements. You made too many calls to $
  • Adding a lot of semicolons which you forgot to use
  • Used plain JavaScript instead of the equivalent jQuery function (attr(), val(), etc.)
  • Stopped relying on id's. You were inserting elements and then using the DOM to retrieve them by ID. I just created elements, then inserted them.

Edit : I just added another closure function because the first wasn't taking $.fb.heftyBox into it's closure.

PS: Setting invalid attributes for a tag is not a good solution. Use 'data-' attributes instead of putting invalid attributes in a tag.

    (function($) {
    $.fn.heftyBox = (function() {
    // usefull functions
    // declared in a self-executing function to not charge the global scope

    var updateheftyBox = function($target) {
        // $target is already a jQuery object, don't need to $() it again
        var box         = $target.data().heftyBox,
            $container  = $target.parent('.heftyBox'),
            $filter     = $target.siblings('.list_filter'); // caching $(filter)

        //Gather created data
        box.value = [];
        $target.find('input:checked').each(function() {
            // using this.value instead of .val(), jQuery tools is not needed
            box.value.push(this.value); 
        });

        $container.css({
            width: box.width,
            height: box.height,
            "min-width": $filter.outerWidth()
        });

        $target.css({
            height: box.height - $filter.outerHeight(),
            "margin-top": $filter.outerHeight()
        })
        $target.val(box.value);
    };


    var filterBox = function($target, blocks) {
        // $target is already a jQuery object, don't need to $() it again

        // use chaining
        $target.unbind('keyup change')
            .bind('keyup change', function() {
                // input, don't need to use $(this).val(), just this.value
                var inText = this.value.trim(); //remove trailing whitespace

                $.each(blocks, function() {
                    var $that = $(this);
                    var title = $that.children('label').text(); //the title in the block
                    if (matchAll(title, inText)) {
                        $that.show();
                    } else {
                        $that.hide();
                    }
                });

            });
    };

    var matchAll = function(string, args) { //string= string to match, args= search input
        var checks = args.split(' '); //break input into array
        for(var i = checks.length; i--; ) {
            if (!string.match(new RegExp(checks[i], 'i'))) { // test if args[i] match the string
                // return false if one don't match, don't need to get threw all
                return false; 
            }
        }
        return true;
    };



    return function(args) {
        var $that = $(this);
        if ($that.length) {
            //Set up defaults
            var opt = $.extend({
                        type   :  "checkbox",
                        width  :  "auto",
                        height :  ($that.innerHeight() > 150) ? $that.innerHeight() : 150
                    }, args
                ),
                attributes = $that[0].attributes, //Gather original attributes, convert DOM, then reassgign attributes
                optionsHTML = $that.html();
                $ul = $('<ul id="tmpul">' + optionsHTML + '</ul>'); // build ul, then use it | maybe use a random id instead of hardcoding one

            for (var i = attributes.length; i--; ) {
                $ul.attr(attributes[i].name, attributes[i].value);
            }


            //Convert options to checkbox or radios
            var options = $ul.children('option'),
                name = $ul.attr('name'),
                f = 0;
            $ul.removeAttr('name'),

            $.each(options, function(key, option) {
                var $that           = $(this),
                    itemAttributes  = $that[0].attributes,
                    value           = $that.value,
                    label           = $that.text(),
                    selected        = itemAttributes.selected ? "checked" : '';

                selected += itemAttributes.disabled ? " disabled" : '';

                var $newLi = $('<li ' + selected + '></li>'), 
                    $input = $('<input type="' + opt.type + '" id="option_' + name + '_' + f + '" name="' + name + '" value="' + value + '" ' + selected + '/><label for="option_' + name + '_' + f + '">' + label + '</label>');
                $newLi.append($input);
                $that.replaceWith($newLi);
                for (var i = attributes.length; i--; ) {
                    $input[0][attributes[i].name] = attributes[i].value;
                }

                f++;
            });

            //Add Filter Box
            var $filter = $('<input id="' + name + '_filter" class="list_filter" />');
            var list = $ul.children('li');
            filterBox($filter, list);

            // add filter and ul to new container
            var $container = $('<div class="heftyBox" id="' + name + '_container"></div>').append($filter, $ul);
            // replace existing element by new container
            $that.after($container).remove();

            //Select all box for checkboxes
            if (opt.type == "checkbox") {
                var $link = $('<a href="#">Select All</a>')
                        .bind('click', function() {
                            var $that = $(this);
                            //@todo get checked
                            // directly using $ul not using specific dom manipulation
                            var $checkboxes = $ul.find('input:not([disabled])');

                            // btw... adding 'checked' attribute to an li is weird... maybe using a data-checked will be better
                            if ($checkboxes.length > $checkboxes.filter(':checked').length) {
                                $checkboxes.attr('checked', 'checked').closest('li').attr('checked', 'checked');
                            } else {
                                $checkboxes.removeAttr('checked').closest('li').removeAttr('checked');
                            }

                            $that.text( ($that.text() == "Select All") ? "Select None" : "Select All");
                            $ul.trigger('change');
                            return false;
                        }
                    );
                // divided creating and appending. just take one more line and is more readable
                $filter.after($link);
            }

            //Write the Data to the DOM
            $ul.data({ heftyBox: opt });

            //Apply DOM data
            updateheftyBox($ul);

            //Handle Value Change
            $ul.bind('change', function() { updateheftyBox($(this)); }) // change event when clickin
                .delegate('input:checkbox', 'click', function() { // delegate click event on checkbox
                    var li = $(this).parent();
                    (this.checked) ? li.attr('checked', 'checked') : li.removeAttr('checked');

                    // trigger change
                    $ul.trigger('change');
                });

            //scroll to first selected DOM
            if ($ul.find('li[checked]:first').length) {
                var itemTop = $ul.find('li[checked]:first').offset().top || $ul.offset().top,
                    ulTop = $ul.offset().top;
                $ul.scrollTop(itemTop - ulTop);
            }
        }

    };
    })();
 })(jQuery);
\$\endgroup\$
  • \$\begingroup\$ Thanks, I copied your code into a separate branch on the GitHub and will try refactoring on my own with the advice I get here, then determine the best end result to merge. \$\endgroup\$ – Kyle Macey Feb 7 '12 at 19:47
2
\$\begingroup\$

Well I forked it and will work on a refactoring but some comments:

  • Cache variables so that you don't have to keep invoking $() - it is common to do var $this = $(this);
  • Don't use ids. They are a host of problems. What if there are 2 hefty boxes on a page? Or worse, what if my app uses one of those names? You virtually never need to do that
  • I prefer using $.each to for loops - they are safer and the performance hit is unlikely to hurt you in this case. In any case you should cache the length since arr.length is recalculated at every invocation.
  • About return false
  • Enclose the same thing in an immediately executing function that makes usage of $ safe

    (function($) {
      // your code
    })(jQuery);
    
  • When you're using jquery to create elements, you don't have to include the closing one.
  • Also, include tests or at least a test page - I will have a hard time testing out my fork.
  • Also I see this pattern a lot:

    $(filter).before('<div class="heftyBox" id="' + name + '_container"></div>'); var container = $('#' + name + '_container');

    This will break if there's another heftybox before this or if they didn't include a name or for a million other reasons. Here is a much better approach:

    var container = $('<div class="heftyBox" id="' + name + '_container"').after(filter);

    Or at he very least use jQuery's next and prev methods.

\$\endgroup\$
  • \$\begingroup\$ jsfiddle here jsfiddle.net/kmacey1249/vXGke \$\endgroup\$ – Kyle Macey Feb 6 '12 at 23:43
  • 1
    \$\begingroup\$ @KyleMacey Updated my answer with a few more items that I saw prevelant. \$\endgroup\$ – George Mauer Feb 7 '12 at 16:09
  • \$\begingroup\$ Thank you very much. This is why I posted here. I've been able to make code work well for specific instances, but needed a good line of correction to nail out best practices and making my code universal. \$\endgroup\$ – Kyle Macey Feb 7 '12 at 17: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.