4
\$\begingroup\$

I have recently adapted the popular leanmodal plugin (with permission). Being "intermediate" with JS (need to learn more), I was wondering if anyone would like to review my code for efficiency.

  • jQuery & JS - bad practice?
  • Functions - structured correctly?
  • Repetition - When using logic, I repeat nearly exact code - bad? Better way to do it?
  • External - $(document).ajax_load(); is external script

Functionality

#Initialize
$('a[rel*=leanModal]').leanModal({
    overlay: 0.9,
    removeModal: true,
    ajax: true
});

Process:

  • Link clicked, leanModal triggered
  • LeanModal checks for "type" of link (ajax / normal)
  • If Ajax OR image, uses special vars
  • If normal, loads overlay & modal
  • Binds click event to over to close
  • Binds click event to close button
  • If user clicks close elements, modal removed

(function( $ ){        

   $.fn.leanModal = function(options) {

        //Defaults
        var defaults = {
            overlay: 0.5,
            closeButton: null,
            delay: null,
            drag: ".modal_title",
            removeModal: null,
            autoLoad: null,
            ajax: false
        };

        //Definitions
        var plugin = this;
        var options = $.extend(defaults, options);

        //Init
        plugin.init = function() {

            if(options.autoLoad){
                $.extend(options, {modal_id: $(this)});
                create();
            }else{
                return this.each(function() {
                    $(this).click(function(e) {
                        e.preventDefault();
                        var href = $(this).attr("href");
                        var image = CheckImg(href);

                        var random = Math.floor(Math.random()*90000) + 10000;
                        var extras = (options.ajax || image) ? {modal_id: "#modal_" + random, ajax: href.replace(/\/$/, '')} : {modal_id: href};

                        $.extend(options, extras);
                        create();
                    });
                }); 
            }
        }

        //////////////////
        //    Actions   //
        //////////////////

        //Build
        var create = function() {
            if(options.ajax){

                //Loading
                Loader();

                //Image
                switch (true) {
                    case CheckImg(options.ajax):
                        append("img", options.modal_id.substring(1), options.ajax);
                        show();
                        break;
                    default:
                        fetch(options.ajax, function(data){
                            append("modal", options.modal_id.substring(1), options.ajax, data);
                            show();
                        }, function(data){
                            Loader();
                            alert("Sorry, there was an error!");
                        });
                        break;
                }

            }else{
                show();
            }
        }

        //Ajax
        var fetch = function(link, success, error) {
            $.ajax({
                url: link,
                success: function(data) { success(data); },
                error: function(data)   { error(data); }
            });
        }

        //Overlay
        var olay = function(modal_id, removeModal, closeButton) {
            var overlay = document.createElement("div");
                overlay.setAttribute("id", "lean_overlay");

            document.body.appendChild(overlay);
            overlay.onclick = function() { close(modal_id, removeModal, $(closeButton)); return false; };
        }

        //Show
        var show = function() {

            /* Vars */
            var id          = options.modal_id
            var removeModal = options.removeModal
            var closeButton = options.closeButton
            var drag        = options.drag
            var ajax        = options.ajax
            var overlay     = options.overlay

            var modal = $(id);
            var overlay = $("#lean_overlay");

            /* Overlay */
            olay(id, removeModal, closeButton);

            /* Options */
            if (closeButton) {
                $(closeButton).css("z-index", "10300");
                $(closeButton).on("click", function (e) {
                    e.preventDefault();
                    close(id, removeModal, $(closeButton));
                    return false;
                });
            }

            /* Loading */
            if (ajax) { modal.load(function() { Loader() }); }

            /* Styling */
            overlay.css({
                    "display": "block",
                    opacity: 0
            });
            modal.css({
                    "display": "block",
                    "position": "fixed",
                    "opacity": 0,
                    "z-index": 10200,
                    "left": 0,
                    "right": 0,
                    "top": 0,
                    "bottom": 0,
                    "margin": "auto"
            });

            /* Init */
            overlay.fadeTo(150, options.overlay);
            modal.fadeTo(200, 1);
            if(drag.length > 0) { modal.draggable({ handle: drag }); }
        }

        //Close
        var close = function(modal, removeModal, closeButton, ajax) {

            if(ajax){ xhr.abort(); }

            $("#lean_overlay").fadeOut(150, function(){
                $(this).remove();
                if(closeButton) {
                    closeButton.off("click");
                    closeButton.removeAttr('style');
                }
            });

            $(modal).fadeOut(150, function(){
                if (removeModal) {
                    $(this).remove();
                 }
            });

        }

        //Go
        plugin.init();
   }; 


    //////////////////
    // Dependencies //
    //////////////////

    var Loader     = function()                     { $(document).ajax_load(); }
    var CheckImg   = function(url)                  { return(url.match(/\.(jpeg|jpg|gif|png)/) != null); }
    var append     = function(type, id, src, data)  {

        //Definitions
        var style = element = type;

        if (type == "modal") {
            var style = "ajax";
            var element = "div";
        }

        //Element
        var el = document.createElement(element);
            el.setAttribute("id", id);
            el.setAttribute("src", src);
            el.className = 'modal ' + style;

        //Ajax
        if (data) { el.innerHTML = data; }

        //Append
        document.body.appendChild(el);
    }

})( jQuery );
\$\endgroup\$
2
\$\begingroup\$

Messy code?

Nah! It really depends on the developer. But I highly suggest you follow certain conventions for clean code, especially the parts regarding indentation, one-liners etc. There are quick tools online for cleaning up code, like JSBeautifier. There are also formatters for code, via Grunt, which formats your code on save (don't know which though, I use a pre-made script).

Never forget ;

Although JS does forgive you for missing ; at certain cases, but in practice, you should never forget them. You will have issues especially when you minify the code.

Poorly created minifiers may not insert ; and you'll end up with something like

`var foo = 'test'var bar='test'`. 

Concat scripts might not merge files and separate them with ;. You'd end up with these at the joining. The compiler might think the previous is a function wrapped in a () and you're trying to execute it (via the second ()) and passing it a function... or something like that. Happens every time I forget to ;.

(function(){
...
}(jQuery))(function(){
   ...
}(jQuery))

switch(true)

This is an odd use for switch

switch (true) {
    case CheckImg(options.ajax):
        append("img", options.modal_id.substring(1), options.ajax);
        show();
        break;
    default:
        fetch(options.ajax, function(data){
            append("modal", options.modal_id.substring(1), options.ajax, data);
            show();
        }, function(data){
            Loader();
            alert("Sorry, there was an error!");
        });
        break;
}

Normally, people would do switch(variable). Though this could work, but it looks weird at first. Not good for usability. Also, one bad thing about switch is the break. Forgetting it would spell disaster. I'd rather go for if-else instead.

if(CheckImg(...)){
    append("img", options.modal_id.substring(1), options.ajax);
    show();
} else {
    fetch(options.ajax, function(data){
        append("modal", options.modal_id.substring(1), options.ajax, data);
        show();
    }, function(data){
        Loader();
        alert("Sorry, there was an error!");
    });
}

Using jQuery? Use it all the way!

I noticed this in your code:

var overlay = document.createElement("div");
    overlay.setAttribute("id", "lean_overlay");

document.body.appendChild(overlay);
overlay.onclick = function() { close(modal_id, removeModal, $(closeButton)); return false; };

I thought you used jQuery? You could have gone with something much more elegant:

$('<div/>',{
  'id' : 'lean_overlay'
}).on('click',function(){
  close(modal_id, removeModal, $(closeButton);
}).appendTo('body');
\$\endgroup\$
  • \$\begingroup\$ Thanks Joseph! I really appreciate all the feedback. RE the semicolons - Ruby / Rails has spoilt me. Switch true was to make it simpler (I had multiple if/else before & didn't like it). Again - thanks +1 \$\endgroup\$ – Richard Peck Mar 1 '14 at 19:36

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.