views:

61

answers:

1

I'm using two plugins I wrote to find all the radio/checkbox inputs and select boxes in a form and them style them.

I now have a large form which a lot of checkboxes and Firefox is hanging as my plugin tries to style each of them.

Here's the plugin code:

(function($)
{
   $.fn.stylecheck = function(options)
   {
      /*
         Parameters:
            image:   the image to load in place of the checkboxes/radio buttons
            offset:  the Y offset (background-position) to show the 'on' position of the image
      */

      return this.each(function()
      {
         if (!$(this).is(':checkbox') && !$(this).is(':radio'))
            return;

         var $input     = $(this);
         var $image     = null;
         var $contianer = null;

         // Wrap the input and then hide it
         $input.wrap('<div class="stylecheck-container" style="display: inline" />').addClass('stylecheck').hide();

         $container = $input.parent('div.stylecheck-container');
         $image     = $container.append('<div class="stylecheck-image" />').children('div.stylecheck-image');

         $image.css(
         {
            "width"   : options['width'],
            "height"  : (options['height'] / 2),
            "display" : "inline-block",
            "background" : ($input.is(':checked')) ? ("url('" + options['image'] + "') no-repeat 0px -17px") : ("url('" + options['image'] + "') no-repeat 0px 0px")
         });

         if ($container.parent('label').length > 0)
         {
            $container.append('<label style="display: inline; position: relative; top: -2px">' + $container.parent('label').text() + '</label> ');
            $container.parent('label').replaceWith($container);
         }

         $input.change(function()
         {
            if ($input.is(':checked'))
               $image.css("background-position", "0px -" + (options['height'] / 2) + "px");
            else
               $image.css("background-position", "0px 0px");
         });

         $container.click(function()
         {
            if ($input.is(':checkbox'))
            {
               if (!$input.is(':checked'))
                  $input.attr('checked', true);
               else
                  $input.removeAttr('checked');
            }

            if ($input.is(':radio') && !$input.is(':checked'))
            {
               $findme = $('input[name="' + $input.attr('name') + '"]:checked')

               if ($findme.length > 0)
                  $findme.each(function() { $(this).attr('checked', false); $(this).trigger('change'); });

               $input.attr('checked', true);
            }

            $input.trigger('change');
         });
      });
   };
})(jQuery);

I'm guessing the problem is with jQuery's each() function searching over hundreds of my checkboxes...

Is there anyway to improve my plugin?

Not all checkboxes are visible on page load (display: hidden). So I'm thinking an alternative will be to style the checkboxes only when they're visibility is toggled - Though, I'd like to leave that as a last resort if my above code can be improved.

Cheers.

+4  A: 

Here's one thing you can improve. You're creating two jQuery objects and calling .is() against both. Then on the next line, you're creating another one and caching it in a variable.

Either cache in the variable before the if() statement, and use the cached version, or ditch jQuery objects for the if() statement altogether, and do something like this:

var type = this.type.toLowerCase();
if (type != 'checkbox' && type != 'radio')
        return;

The rest here will be documentation of @Nick Craver's posted jsFiddle.

Overall, don't use jQuery when you can avoid it. It is simply faster to use the native API. When you do use jQuery, use it in the most minimal manner possible.

You can change this line:

$container = $input.parent('div.stylecheck-container');

to this:

$container = $input.parent();

since you wrapped the $input, no need to test the parent with a selector.

Change this line:

"background" : ($input.is(':checked')) ? ("url('" + options['image'] + "') no-repeat 0px -17px") : ("url('" + options['image'] + "') no-repeat 0px 0px")

to this in order to avoid a call to .is(). Doing this.checked returns a boolean value:

"background" : this.checked ? ("url('" + options['image'] + "') no-repeat 0px -17px") : ("url('" + options['image'] + "') no-repeat 0px 0px")

In the handlers you assign, change $input.is('checked') to $input[0].checked. This gets the DOM element, and gets the checked attribute. This won't speed up the plugin execution, but will improve the handlers.

Also, change $input.is(':checkbox') to $input[0].type == "checkbox" (and likewise with radio). You could even cache the type in a variable as I did at the top of this answer, and use that value. As in type == "checkbox".

patrick dw
I didn't see @Nick's comment below the answer. This is very similar to what he did in part of his solution.
patrick dw
+1 - There are other improvements to be had in there as well, mostly around DOM property and traversal reduction. I don't have time to do a full write-up, but feel free to use the version I posted in comments as a reference/inclusion if you have more motivation than I to explain it :)
Nick Craver
Thanks @Nick. I'll document your changes here. :o)
patrick dw
I've changed my code to what Nick supplied. Then modified that to use your 'cache the type' suggestion. Are there any other tweaks you could suggest?
Sam
@Sam - You could try getting rid of the `$image.css(...)` call, and just adding those style attributes inline when you create the element. Reduces one more call.
patrick dw
@Sam - ...also, this probably wouldn't make much difference, but you can get rid of comparison operators by replacing code like this `if($container.parent('label').length > 0)` with `if($container.parent('label').length)`.
patrick dw
I've made all the changes you suggested. Cheers.
Sam
@Sam - You should accept this as the answer that helped via the check-mark on the left :)
Nick Craver