Dialog multiple instantiations

Question

I have a site where there are lots of "jobs" that a user creates. The user has the option to Close any given job. I want a pop up warning dialog whenever the user clicks the Close button, and verify that they actually want to close the job.

There are many ways to do this, and I've implemented it this way (jsfiddle):

function openCloseDialog(item) {
    var jobid = $(item).attr("jobid");
    var closeLink = $(item).attr("value");

    if ($("#closeWarningModal").size() > 0) {
        $("#closeWarningModal").dialog({
            autoOpen: true,
            'modal': true,
            title: "Close Job",
            draggable: true,
            resizable: true,
            'buttons': {
                'Cancel': function () {
                    $(this).dialog('close');
                },
                'Close': function () {
                    // close 
                    $(this).dialog('close');
                },
            }
        });
    } else {
        // close 
    }
}

This seems to be working perfectly fine. The question I have is, is there any reason to NOT do it this way? At first I was curious if this implementation would cause multiple instantiations of the dialog, or if the user clicked the Close button a second time it would fail to open. But neither of those things are happening and it seems to work perfectly.

Can you see or find anything wrong with this code?

Answer 1

Looks OK to me. jQueryUI will take care of most of the low-level stuff like not letting you open several dialogs and such (by simply not letting you click "behind" the modal).

The code could be cleaned up though:

• Store $(item) in a variable (same with the modal element)
• No need to quote those option names
• Most of the options you're setting are already the default options
• Extract repeated code into functions
• Don't use plain attributes to store "jobid" and certainly not "value" (since that's a real attribute for some elements). It's invalid markup. Use data-* attributes instead.
• Don't use the onclick attribute either. You have jQuery: Use it.
• Consider using a more semantically appropriate element for the close-job button. Like <a> or <button>. A div is semantically void; it doesn't mean anything. An a or a button, meanwhile, implies that it's clickable and will perform some sort of a action.

JavaScript

$('.close-job').on('click', function (event) {
  var button = $(this),
      job = button.data('job'),
      uri = button.data('uri'),
      modal = $('#closeWarningModal');

  function closeJob() {
    // ...
  }

  function dismissModal() {
    modal.dialog('close');
  }

  if(modal.length) {
    modal.dialog({
      modal: true,
      title: 'Close Job',
      buttons: {
        Cancel: dismissModal,
        Close: function () {
          closeJob();
          dismissModal();
        } // <-- there was an extra comma here; some versions of IE will choke on that
      }
    });
  } else {
    closeJob();
  }
});

HTML

<button class="close-job" data-uri="closeurl" data-job="jobid">Close</button>