7
\$\begingroup\$

I'm trying to extend bootstrap 3 modal window with ability to disable closing it.

I've built this code:

(function ($, window, document, undefined) {
    var oldHide = $.fn.modal.Constructor.prototype.hide;

    $.fn.modal.Constructor.prototype.hide = function (_relatedTarget) {
        if (this.isLocked) return;
        return oldHide.call(this, _relatedTarget);
    };

    $.fn.modal.Constructor.prototype.lock = function (_relatedTarget) {
        this.isLocked = true;
        e = $.Event('lock.bs.modal', {
            relatedTarget: _relatedTarget
        });
        this.$element.trigger(e);
    };

    $.fn.modal.Constructor.prototype.unlock = function (_relatedTarget) {
        this.isLocked = false;
        e = $.Event('unlock.bs.modal', {
            relatedTarget: _relatedTarget
        });
        this.$element.trigger(e);
    };

})(jQuery, window, document);

Working demo

After clicking "lock," you won't be able to close modal until you click "unlock."

It this way of extending bootstrap modal correct? What are potential dangers of doing this?

And how can I make this better?

EDIT:

This is my final version based on suggestions.

\$\endgroup\$
  • \$\begingroup\$ I get passing in this in case someone overwrite window, but what pass in window and document only to assign them to function arguments with the same name? Does that have any effect? \$\endgroup\$ – David Harkness May 29 '14 at 2:21
  • \$\begingroup\$ @DavidHarkness - I found template to write extends, so I build my code based on it. Because I wasn't sure if this is correct I post my code here, to get some comments from pros :) \$\endgroup\$ – Misiu May 29 '14 at 5:59
  • \$\begingroup\$ @DavidHarkness the idea behind doing that kind of thing is that resolving a variable declared in an "inner" scope should be more efficient than resolving one from an "outer" scope, since the innermost scope is checked first, then the one outside that, and so on. Micro optimization at its finest. BTW, I don't think any implementation actually allows window to be overwritten. \$\endgroup\$ – Dagg Jun 3 '14 at 9:33
  • \$\begingroup\$ @Dagg so I should leave (jQUery, document, window) in my code? or is (jQuery) fine? \$\endgroup\$ – Misiu Jun 3 '14 at 15:27
  • \$\begingroup\$ @Misiu is this all of your code? It never references window, document, or undefined, so... \$\endgroup\$ – Dagg Jun 3 '14 at 18:13
8
\$\begingroup\$

Looks fine to me, except that you failed declare the e variable in the lock/unlock functions. So it's an automatic global, which isn't good.

With a little bit of cleanup:

(function ($) {
  var _original = $.fn.modal.Constructor.prototype.hide;

  function trigger(target, name, relatedTarget) {
    target.trigger($.Event(name, { relatedTarget: relatedTarget }));
  }

  $.extend($.fn.modal.Constructor.prototype, {
    hide: function (_relatedTarget) {
      if (this.isLocked) return;
      return _original.call(this, _relatedTarget);
    },

    lock: function (_relatedTarget) {
      this.isLocked = true;
      trigger(this.$element, 'lock.bs.modal', _relatedTarget);
    },

    unlock: function (_relatedTarget) {
      this.isLocked = false;
      trigger(this.$element, 'unlock.bs.modal', _relatedTarget);
    }
  });
})(jQuery);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I accepted this answer because it cleans globals. I didn't saw that problem until I read this answer. \$\endgroup\$ – Misiu May 29 '14 at 6:14
  • \$\begingroup\$ @Misiu If you find yourself suspecting your code may have issues like implicit globals (or even if you don't), it's always helpful to run your code through a linter like jslint or jhint which'll catch such stuff. And thanks :) \$\endgroup\$ – Flambino May 30 '14 at 0:30
6
\$\begingroup\$

Flambino did an excellent review, the only nitpick I have is that I would write

$.fn.modal.Constructor.prototype.hide = function (_relatedTarget) {
    if (this.isLocked) return;
    return oldHide.call(this, _relatedTarget);
};

as

$.fn.modal.Constructor.prototype.hide = function (_relatedTarget) {
    if (!this.isLocked){
      return oldHide.call(this, _relatedTarget);
    }
};
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for tweaking my code :) but as I wrote below @Flambino's answer I did't realize that I have potential global leaks. His answer cleaned that problem. \$\endgroup\$ – Misiu May 29 '14 at 6:16
  • \$\begingroup\$ @Misiu No worries, Flambino's review is far more in depth. \$\endgroup\$ – konijn May 29 '14 at 10:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.