Each function not working as expected when using custom function

Question

Consider this code running on page ready:

$("input.extraOption[checked]").each(function() {   
        console.log($(this));
        $(this).closest('.questionRow').find('.date').attr("disabled", true);
        $(this).closest('.questionRow').find('.dateSpan').hide();
        $(this).closest('.questionRow').find('.date').val("");
        $(this).closest('.questionRow').find('.textareaResize').attr("disabled", true);
        $(this).closest('.questionRow').find('.textareaResize').val("");
        $(this).closest('.questionRow').find('.text').attr("disabled", true);
        $(this).closest('.questionRow').find('.text').val("");
        $(this).closest('.questionRow').find('.checkbox').attr("disabled", true);

    });

I want to refactor these calls as they are used elsewhere as well, so I created the following function:

jQuery.fn.extend({
    toggleAnswers: function (disable) {
        var group = $(this);
        group.find('.date').attr("disabled", disable);
        group.find('.date').val("");
        group.find('.textareaResize').attr("disabled", disable);
        group.find('.textareaResize').val("");
        group.find('.text').attr("disabled", disable);
        group.find('.text').val("");
        group.find('.checkbox').attr("disabled", disable);
        if(checkedStatus === true){
            group.find('.dateSpan').hide();
        }else{
            group.find('.dateSpan').show();
        }
    return group;
    }
});

I then proceed to changing the 8 $(this).closest(...) calls with:

$(this).closest('.questionRow').toggleAnswers(true);

Here's the problem: on a page with 5 elements that match the selector, only the first one suffers the changes (in other words I only get one console.log)! Before the refactor I get the expected change in all 5 elements.

What is being done wrong in this refactor?

2 random things - `this` inside of a plugin is already a jQuery object, so you shouldn't need to use `$(this)` - just `var group = this;` (if you must). Also, `checkStatus` doesn't seem to be defined... It would probably be a lot easier for us to help if you provided a jsFiddle — Ian, Aug 22 '13 at 18:25
Yes, I was just about to edit this - I just found out that checkStatus should be "disable". That's what was causing the problem! — mmalmeida, Aug 22 '13 at 18:26
@Ian -since you noticed, you can answer it and I'll accept the answer. — mmalmeida, Aug 22 '13 at 18:27
Also it'd be a really good idea to avoid having to do two separate `.find()` calls for each type of input to affect. After setting the "disabled" property (and I'd use `.prop()` instead of `.attr()`, not a big deal), you can call `.val("")` on the return value of that. Chaining is a good thing to do when you can. — Pointy, Aug 22 '13 at 18:28
You should add an iterator within the function body, so it will work against more than one matched element, too. That's standard behavior for a jQuery plugin... observe: http://jsfiddle.net/GRMule/SX7jF/1/ — Chris Baker, Aug 22 '13 at 18:30
@Chris jQuery setter methods do that internally, so it's unnecessary. The use of `.each` is only necessary if you want to do specific things on specific matched elements. In this case, the OP is doing the same thing to all matched — Ian, Aug 22 '13 at 18:32
Using an iterator in a plugin body is recommended right in the docs: http://api.jquery.com/jQuery.fn.extend/ -- it isn't necessary, but it keeps the behavior of plugins consistent. Meh. I'd do it in my code. — Chris Baker, Aug 22 '13 at 18:35
@Chris I agree it should be used and I also encourage consistency. But you didn't say that. Your point was about needing to iterate in order to access each, which wasn't true/necessary. That was all I meant — Ian, Aug 22 '13 at 18:37

score 1 · Accepted Answer · edited May 23 '17 at 12:05

checkStatus isn't defined anywhere, causing an exception. You seem to want to use disable instead.

On a side note, this already refers to the jQuery collection that this method is called on, so wrapping this in a jQuery object ($(this)) is redundant/unnecessary. Note that this is specifically inside of a $.fn method, not normal jQuery methods. For example, inside event handlers, this refers to the DOM element, so you need to wrap it in $(this) in order to call jQuery methods on it.

Also, disabling an element should be done with .prop("disabled", true/false): .prop() vs .attr()

You can also combine any selectors that you call the same jQuery method on. For example, group.find('.date').val(""); and group.find('.text').val(""); can be combined into: group.find(".date, .text").val("");

Putting all of those suggestions together, as well as iterating over this (for consistency and scalable sake), here's what I'd use:

jQuery.fn.extend({
    toggleAnswers: function (disable) {
        return this.each(function (idx, el) {
            var $group = $(el);
            $group.find(".date, .text, .textareaResize, .checkbox").prop("disabled", disable);
            $group.find(".date, .textareaResize, .text").val("");
            $group.find(".dateSpan").toggle(!disable);
        });
    }
});

And depending on how you use it, I'd set it up like:

var targets = $("input.extraOption[checked]"),
    toggler = function () {
        $(this).closest(".questionRow").toggleAnswers(this.checked);
    };

targets.each(toggler).on("click", toggler);

DEMO: http://jsfiddle.net/XdNDA/

Each function not working as expected when using custom function

1 Answers1