Skip to main content
tweaks, addenda
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90

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 propertyoption 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>

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 property 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). 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 button. Like <a> or <button>.

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>

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>
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90

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 property 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). 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 button. Like <a> or <button>.

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>