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
– Ian, Commented Aug 22, 2013 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
– mmalmeida, Commented Aug 22, 2013 at 18:26
@Ian -since you noticed, you can answer it and I'll accept the answer. — mmalmeida
– mmalmeida, Commented Aug 22, 2013 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
– Pointy, Commented Aug 22, 2013 at 18:28
@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
– Ian, Commented Aug 22, 2013 at 18:32

Community · Accepted Answer · 2017-05-23 12:05:35Z

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/

Collectives™ on Stack Overflow

Each function not working as expected when using custom function

1 Answer 1

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related