What's Wrong With This Code? (#28)

Monday, August 15, 2011

Victor Brumble is writing code again, and this time he's writing a jQuery plugin. His plugin makes alert boxes easy, because Victor loves pointless, modal dialogs.

(function ($) {

    var settings = {
        text: 'Thank you for clicking!'
    };
    
    $.fn.alerter = function (options) {

        return this.each(function () {

            if (options) {
                $.extend(settings, options);
            }

            $(this).click(function () {
                alert(settings.text);
            });
        });
    };
})(jQuery);

As you can see, Victor has a default text message associated with his plugin, but he also learned how to use jQuery.extend to allow callers to override the default text. The plugin is working well for Victor, but another developer is complaining about the following code not working correctly.

<p id="p1">
    This is paragraph 1.
</p>
<p id="p2">
    This is paragraph 2.
</p>
<script type="text/javascript">
    $("#p1").alerter({ text: "Please don't click again!" });
    $("#p2").alerter({ text: "This is not the text you want!" });
</script>

What could possibly be wrong?


Comments
gravatar Mike Monday, August 15, 2011
the settings object is going to be shared between the two instances since it is declared outside the each().
Kenneth Monday, August 15, 2011
$.fn.alerter = function (options) {

return this.each(function () {

var newSettings = $.extend({}, settings, options);

$(this).click(function () {
alert(newSettings.text);
});
});
};

gravatar Peter Bromberg Monday, August 15, 2011
Victor needs some tutelage in grammar too. It's "MODAL", not "MODEL" dialogs that he likes.
gravatar Brent Anderson Monday, August 15, 2011
(function ($) {
$.fn.alerter = function (options) {
var settings = {
text: 'Thank you for clicking!'
};
if (options) {
$.extend(settings, options);
}
return this.each(function (index,item) {
$(item).click(function () {
alert(settings.text);
});
});
};
})(jQuery);
gravatar scott Monday, August 15, 2011
@PB - thanks for catching that :)
gravatar Peter Bromberg Monday, August 15, 2011
Mark Bennett made a jsFiddle of this.
http://jsfiddle.net/d7WHx/2/
gravatar Harry Tuesday, August 16, 2011
Mike and Kenneth are right. The setting variable is shared and overwritten.

http://jsfiddle.net/harrychou/Lhj49/
gravatar Harry Tuesday, August 16, 2011
gravatar Duncan Smart Tuesday, August 16, 2011
Should `this.each` be `$(this).each`?
gravatar Duncan Smart Tuesday, August 16, 2011
Nope, scratch that :-) `this` is a jQuery object in a plugin (at that initial scope)...
kipper mcslice Monday, September 5, 2011
#{aka (...)} t,"" dun duda duh
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!