current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite

I want to create an extended disable/enable function in jQuery. This is working, but is this the best solution? Is each loop necessary?

 $.fn.ariaDisabled = function (isDisabled) {
        /// <summary>
        ///     Sets the disabled state.
        /// </summary>
        return this.each(function () {

            var $item = $(this);

            if (isDisabled === true) {
                $item
                    .attr('disabled', 'disabled')
                    .attr('aria-disabled', 'true')
                    .addClass('disabled');
            }
            else {
                $item
                    .removeAttr('disabled')
                    .removeAttr('aria-disabled')
                    .removeClass('disabled');
            }
        });
    };
share|improve this question
    
If the function is called ariaDisabled then it should only change the [aria-disabled] attribute. –  zzzzBov Jan 12 at 15:18

1 Answer 1

up vote 4 down vote accepted

Overall it looks quite nice. Just a few points:

  • The parameter name is asking a question about the element, not making a statement of what to do. I would rename it to disable which enables the caller to better signify their intent.
  • In the same vein, I would also rename the function to ariaDisable as it performs an action as opposed to asking a question

Edit

Per m90's comment the loop is per jQuery plugin conventions as noted here

share|improve this answer
2  
+1 but you should return $item in the fiddle to support chaining. –  RobH Jan 12 at 12:42
    
Thanks @RobH. I've fixed that –  Max Jan 12 at 12:50
2  
The return this.each(fn) loop is a standard convention when writing jQuery plugins though. I'd recommend keeping it for a situation (unlike the fiddle) where you cannot perform all operations on the collection at once. It also saves you some head scratching on why this is a jQuery-object already. learn.jquery.com/plugins/basic-plugin-creation/… –  m90 Jan 12 at 15:57
1  
thanks @m90 for that. I have updated my answer –  Max Jan 12 at 16:07

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.