2
\$\begingroup\$

This is my first attempt at a jQuery plugin, which is a simple modal window. I'm keen for feedback of whether I've got the principle correct, the usability of the plugin and any improvements that could be made.

Here is a working JSBin.

The plugin instructions:

  1. Add one or more elements that will launch a modal when clicked. Give these a class of modal-open.

  2. Add elements to display in the modal window. These should go in a </div> with class="overlay-message hide". and the div needs an id.

  3. Add a data-target attribute to the modal-open element(s) made in 1. The value of this should be the same as the id of the overlay-message element made in 2.

  4. Add links to jQuery, overlay.js and overlay.css

  5. Call $('.modal-open').modalise()

The JS code:

(function ( $ ) {

$.fn.modalise = function( options ) {

    // Add and initialise the overlay background 
    $('body').append('<div class="overlay modal-close hide"></div>');
    $('.modal-close').on('click',function(){
        $('.overlay').toggleClass("hide");
        $('.overlay-message').addClass("hide");
        console.log('clicked'); 
    });

    // Options for the plugin
    var settings = $.extend({
        // Default options
        width: "700px",
        closeButton: true
    }, options );

    return this.each(function(index){
        // Dom elements
        var openButton = $(this),
            targetSelector = '#' + $(this).attr('data-target');

        // Add and initiate close button
        if(settings.closeButton){
            $(targetSelector).prepend('<a href="#" class="modal-close btn">X</a>').find('.modal-close').on('click',function(){
                $('.overlay').addClass("hide");
                $('.overlay-message').addClass("hide"); 
            });
        }

        // Show the relevant modal window on clicking the open button
        openButton.on('click',function(){
            var targetSelector = '#' + $(this).attr('data-target');
            $(targetSelector + ', .overlay').toggleClass("hide");
        });

    });

};

}( jQuery ));

The CSS code:

.overlay{
    width: 100%;
    height: 100%;
    background: #000000;
    filter: alpha(opacity=80);
    background: rgba(0,0,0,0.8);
    position: fixed;
    top: 0;
    right: 0;
    bottom: 0;
    left: 0;
    z-index: 1000;
}
.overlay-message{
    padding: 20px;
    background: white;
    position: absolute;
    width: 660px;
    left: 50%;
    margin-left: -350px;
    top: 10%;
    z-index: 10000;
}
.hide{
    display: none;
}
.modal-close.btn{
    position: absolute;
    right: 8px;
    top: 5px;
    color: black;
    background: none;
    text-decoration: none;
}
.modal-open{
    cursor: pointer;
}   

@media only screen and (max-width: 767px){
    .overlay-message{
        width: auto;
        left: 5%;
        right: 5%;
        margin-left: 0;
    }
}
\$\endgroup\$
1
\$\begingroup\$

To avoid a lot of issues in the future I can recommend writing your plugin in "use strict"; mode. Next improvement could be using variables at the beginning of your function to make it faster and cache values. For instance:

$.fn.modalise = function( options ) {
  "use strict";

  var root = $(".body"),
      overlay = $(".overlay"),
      overlayMessage = $(".overlay-message"));

   [...]
}(jQuery));

Because each time your plugin is called, JavaScript will search through the DOM for $(".body") and all other elements.

Edit: And you allready cached $(this), why don't you use it?

return this.each(function(index){
  // Dom elements
  var openButton = $(this),
      targetSelector = '#' + openButton.attr('data-target'),
      closeModal

  // Add and initiate close button
  if(settings.closeButton){
    $(targetSelector).prepend('<a href="#" class="modal-close btn">X</a>').find('.modal-close').on('click',function(){
      overlay.addClass("hide");
      overlayMessage.addClass("hide"); 
    });
  }

I guess there are some more improvements you could make. I'm just not an expert in writing own plugins :-)

\$\endgroup\$
  • \$\begingroup\$ I can not see how html structure.however, in my opinion, when you defined root, then all other module's objects should use root.find() to find, not $(). \$\endgroup\$ – caoglish Apr 10 '14 at 1:19
  • \$\begingroup\$ I missed that another element is overlay, however, overlay is should be only-one element for this module, so it may cache to store it and use the cache variable cross the this module object, rather than search it all the time \$\endgroup\$ – caoglish Apr 10 '14 at 1:23
  • \$\begingroup\$ Thanks for the feedback. Ive made some changes based on that. \$\endgroup\$ – icottam91 Apr 10 '14 at 11:14
  • \$\begingroup\$ You're welcome. Furthermore you could use JSLint on your plugin which will analyse your code and display potential issues. \$\endgroup\$ – morkro Apr 10 '14 at 11:33

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.