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 1 down vote favorite

The goal of the code is to

  • initialize a Dialog with settings
    • make sure the object is reusable
  • Setup the options to reuse the code

I'm not quite sure if this is overkill, or I am going about this problem in the entirely wrong way. Any input would be appreciated.

var myDialog = myDialog || (function() {

  var options = {
    title: 'Attention',
    msg: 'Message Here',
    focusOn: ''
  }

    function show(opts) {

      options = $.extend({}, options, opts);

      $('<p>' + options.msg + '</p>').dialog({
        title: options.title,
        modal: true,
        draggable: false,
        show: {
          effect: "fade",
          duration: 500
        },
        hide: {
          effect: "fade",
          duration: 200
        },
        buttons: {
          'Close': function() {
            $(this).dialog("close");
          }
        },
        close: function() {
          if (options.focusOn.length !== 0) {
            $(options.focusOn).focus()
          }
          $(this).dialog("destroy");
        }
      });
    }

    function updateOptions(opts) {
      options = $.extend({}, options, opts);
    }

  return {
    fire: show,
    options: updateOptions
  };

}());

myDialog.fire({
  msg: 'Hello there',
  focusOn: '#test'
});
share|improve this question

1 Answer 1

up vote 1 down vote accepted

It is already a good attempt at making better-looking code. Just a few improvement:

  1. Declare all private functions one-by-one with declaring that individual just like you declare your variable (see jQuery code).

  2. There is no need for the updateOptions() method; we don't need it.

var myDialog = myDialog || (function () {

    var options = {
        title: 'Attention',
        msg: 'Message Here',
        focusOn: ''
    }

    var show = function(opts) {
            options = $.extend({}, options, opts);
            $('<p>' + options.msg + '</p>').dialog({
                title: options.title,
                modal: true,
                draggable: false,
                show: {
                    effect: "fade",
                    duration: 500
                },
                hide: {
                    effect: "fade",
                    duration: 200
                },
                buttons: {
                    'Close': function() {
                        $(this).dialog("close");
                    }
                },
                close: function() {
                    if (options.focusOn.length !== 0) {
                        $(options.focusOn).focus();
                    }
                    $(this).dialog("destroy");
                }
            });
        },

        updateOptions = function(opts) {
            options = $.extend({}, options, opts);
        };

    return {
        fire: show,
        options: updateOptions
    };

}());
share|improve this answer
    
Is this a correct approach, or is there a better way to 'initialize' .dialog()? Additionally, could you please point me to where you mean by 'see jquery code'. –  Brett Santore Jul 30 '14 at 12:36
    
this is right way to use to it , jquery.com/download look into any of the version , you will find out how they are using –  paritosh Jul 30 '14 at 13:05

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.