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.

I've created my first jQuery plugin, so please try to understand :)

My plugin code looks like so:

(function($) {
$.fn.ajaxSelect = function(options) {
    var $this = this;
    //options
    var settings = $.extend({}, defaults, options);
    //disable select
    if ($.ui.selectmenu && settings.selectmenu && settings.disableOnLoad) {
        $this.selectmenu('disable');
    }
    //ajax call
    $.ajax({
        type: settings.type,
        contentType: settings.contentType,
        url: settings.url,
        dataType: settings.dataType,
        data: settings.data
    }).done(function(data) {
        var n = data.d || data;
        var list = "";
        $.each(n, function(i) {
            list += '<option value=' + n[i].Id + '>' + n[i].Nazwa + '</option>';
        });
        $this.filter("select").each(function() {
            $(this).empty();
            $(this).append(list);
            if ($.ui.selectmenu && settings.selectmenu) {
                $this.selectmenu();
            }
        });
        settings.success.call(this);
    }).fail(function() {
        settings.error.call(this);
    });

    return this;
};

var defaults = {
    type: "POST",
    contentType: "application/json; charset=utf-8",
    url: '/echo/json/',
    dataType: 'json',
    data: null,
    async: true,
    selectmenu: true,
    disableOnLoad: true,
    success: function() {},
    error: function() {}
};
})(jQuery);

Demo of usage is available at http://jsfiddle.net/Misiu/ncWEw/

What I was trying to accomplish:

  • Select multiple elements at one time
  • Filter only selects from selected items (in case selector would select other elemnts)
  • Makes only ONE request to server
  • First build option string and then append it instead of adding items in loop
  • Specify 2 callbacks: one for error and second for success

I think that it would be better to use more of deferred objects, but I don't have idea how. Any improvements and hints are welcome :)

share|improve this question

1 Answer 1

up vote 3 down vote accepted

Your demo throws errors in console, something about uniqueId in ui.selectmenu.

Anyway:

  1. You can expose $.ajax deferred so someone may use all features of jQuery.Deferred which is already there.
  2. Call success callback with data as argument.

    settings.success.call(this, n);
    
  3. If you do not want to do anything special in case of failure, and just want to provide this to error callback, you do not have to create closure there.

    .fail( settings.error );
    

    ..., unless you want to hide other rejection arguments.

  4. Do not use each if for do the same (for performance reason):

    for( var index = 0; index < n.length; index ++){
      list += '<option value=' + n[index].Id + '>' + n[index].Nazwa + '</option>';
    }
    
  5. Personally I don't like single char variable names, as they are really painful for future development

  1. Instead of $(this).empty();$(this).append(list); you can simply use $(this).html(list);

  2. You can expose jQuery.Deffered, for example as this.promise = $.ajax(..., example in here http://jsfiddle.net/dhRRN/5/

  3. I would suggest also applying received options to ajax call, so your plugin's user could also alter request itself.

share|improve this answer
    
Thanks for all the tips :) I've updated my demo to remove all errors, and I changed each to for. Could You please show how should I change my code to use Deferred? I suppose I could add data to success callback, but in my case I wont need it later. Could You take a look at it again? –  Misiu Apr 16 '13 at 8:02
    
Regarding adding/hiding data for callback, I have added new #3. @Misiu: I'm not exactly sure how are you going to use your plugin. Here you have quick example how to expose jQuery.Deferred promise. –  tomalec Apr 16 '13 at 18:04
    
I'll be using this mostly in reports. For example I need to filter some data based on employees. I have 4 places to specify employee (employee that took order, one that collect all items in order, one that shipped etc) and the list I can choose is the same for every select. So my main idea was to fill multiple selects with same data that comes from ajax request and to do only one request for all selects. This is just a scratch rather than full working plugin, but I would like to create something usefull not only for me :) –  Misiu Apr 17 '13 at 6:51

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.