My first jQuery plugin -- Heftybox

Question

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/

• dievardump: 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.
• Kyle Macey: Updated with example

Answer 1

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);

• Kyle Macey: 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.

Answer 2

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.

• Kyle Macey: jsfiddle here jsfiddle.net/kmacey1249/vXGke
• George Mauer: @KyleMacey Updated my answer with a few more items that I saw prevelant.
• Kyle Macey: 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.