0
\$\begingroup\$

I have coded my own lightbox for the sake of my own custom needs because I want to be able to do this kind of thing myself and learn all the various things involved. I realize I could use a plugin, but that is not the point here.

Last week I posted this vague question to get some feedback on the style, and ultimately the code I am posting here now is really a much better question I think:

Is my method of making the lightbox an object filled with different independent methods valid?

  • I like doing it this way because it allows me to make a bunch of isolated individual functions that I can call on and reference when
    needed.
  • When I need to modify part of the process, most things are isolated and easy to work on.
  • Sometimes I may have difficulty drawing the line between separating functions and keeping them combined for simplicities sake.

As you look at the code you might want to say "Why didn't you do this, or why didn't you do that" Or you might say "Why are you doing it like this" to which I will say, that will not be constructive for me. I am doing it this way because I have found it to be a nice way of doing things and frankly I am not as adept at writing the code other ways. I have been on a JS/Jquery crash course for about two years now trying to make the best I can of it.

var lightbox={
      start: function(){
          $('body').on('click','img',function(){
              var target = $(this).attr('src');
              lightbox.createScreen();
              lightbox.getDimensions();
              lightbox.createImageBackDrop(target);
              lightbox.listenForClose();
          });
      },
      createScreen: function(){
              var $screenElement = $('<div></div>').attr('id','modalscreen')
               .css(
                     {
                       position:'absolute',
                       width:'100%',
                       height:'100%',
                       zIndex:10000,
                       backgroundColor: '#000000',
                       opacity: '0.8'                                      

                    }     

                )
                lightbox.prependScreen($screenElement);
        },
       prependScreen: function($e){       
          ($e).prependTo(document.body);
       },
       createImageBackDrop: function($image){
          var $backDrop = $('<div></div>').attr('id','imageback')
              lightbox.sizeBackDrop($backDrop,$image);
       },
       sizeBackDrop: function($e,src){
           //original height / original width x new width = new height
           var width = lightbox.getDimensions('x');
           var imgWidth = Math.round(width*.52);
           var imgHeight = Math.round(750/1000*imgWidth);
           var bdWidth = imgWidth+20;
           var bdHeight = imgHeight+20;
           var bdMarginTop = -Math.abs(bdHeight/2);
           var bdMarginLeft = -Math.abs(bdWidth/2);

             $e.css(
                    {
                         position:'absolute',
                         top:'50%',
                         left:'50%',
                         marginTop:bdMarginTop,
                         marginLeft:bdMarginLeft,
                         width: bdWidth+'px',
                         height:bdHeight+'px',
                         zIndex:10001,
                         backgroundColor: '#FFF',
                         padding:'10px'
                    }
                 )
                lightbox.prependBackDrop($e);
                lightbox.insertImage(src,imgWidth,imgHeight);
       },
       insertImage: function(src,w,h){
            var imgMarginTop = -Math.abs(h/2);
            var imgMarginLeft = -Math.abs(w/2);
            var $image = $('<img>')
                  .attr({
                        'src':src,
                        'height':h,
                        'width':w
                        })
                  .css({
                     position:'absolute',
                     top:'50%',
                     left:'50%',
                     marginTop:imgMarginTop,
                     marginLeft:imgMarginLeft,
                 });

          ($image).appendTo('#imageback');

       },
       prependBackDrop: function($e){
          ($e).prependTo(document.body);

       },
       getDimensions: function(e){
           var width = screenwidth.checkSize();
           var height = screenwidth.checkHeight();
           if(e == 'y'){
               return height;
           }
           else if (e == 'x'){
               return width;
           }
       },
       listenForClose: function(){
           $('body').on('click','#modalscreen',function(){
           $('#modalscreen').remove();
           $('#imageback').remove();
           });

       }
}
//end lightbox

$(document).ready(screenwidth.start); //another object that returns screen size info
$(document).ready(lightbox.start);
\$\endgroup\$
3
\$\begingroup\$

First of all, this may be a matter of opinion (my CTO totally disagrees on this, for instance), but I think splitting a long method / function into tiny ones with a single, simple purpose is the right way of doing things.

Actually, this is also the position held by the "Clean Code" book and many people out there.

The benefits you state do exist, and other reasons to code this way can be found in aforementioned book. What i really like about this though :

  • your code becomes self-documenting, so that it's easier for other people to understand what you intend to do.
  • it helps to get an overview of a complex process without having to deal with all the low-level details, except if you want it.
  • Perhaps more importantly, it helps scoping things (for example, in long functions you sometimes have to crawl all the way up because you don't remember what this or that var is, in which state it is, etc.).
  • I also find it helps reasoning about the problem you're dealing with, because with this method you tend to split the problem into smaller problems, which is about the very essence of programming.

That said, I have a few pointers that you might consider helpful.

Use closures

Your lightbox object is not really necessary, and sometimes it obfuscates the code a bit.

In fact, as your script only evaluates once (no subsequent call after start), you could replace your object with a closure :

 (function(){

    // initialize stuff

 })();

What happens here is that you create a function that you immediately call ; all vars in the function body are in a local scope that is not accessible from the outside (this prevents conflicts with other scripts).

Using a closure, you will be able to run intialization code without having to stuff all your functions inside an object or worrying to pollute the global namespace :

// the bang! is just another way to create a closure... 
// with one less character to type (closing parens)
!function(){

  // I like to put init stuff on top, this is debatable
  doThis();
  doThat();
  doThose();

  // these functions are local to the scope ;
  // they are also callable from anywhere in this scope 
  // (search 'javascript variable lifting' for more info)
  function doThis(){}; 
  function doThat(){};
  function doThose(){};

}();

Use closure-local variables

In your code, your init stuff is called every time the user clicks on an image. This is bad because, for instance, you call createScreen every time, creating a new element an appending it to body over and over. As you will only need one screen throughout the entire app lifecycle, it would be sensible to do something like this :

!function(){

  // we will only create this once, and operate on this only element
  var screen = createScreen();

  function createScreen(){ 
    //... create your element 
  };

  function showScreen(){
    screen.prependTo(document.body);
  };

  // remove() only removes the element from the DOM,
  // it remains usable and accessible through the var
  function hideScreen(){
    screen.remove();
  }

}();

You can even get a bit more object-oriented on this one :

!function(){

  // ad-hoc object, could be a real "class" with elaborate 
  // constructor and prototype
  var screen = {
    element: createScreen(),
    show: function(){ this.element.prependTo(document.body); },
    hide: function(){ this.element.remove(); }
  };

  $(document)
    .on('click','img', open)
    .on('click','#modalscreen', close);

  function open(){
    screen.show();
    // insert image and stuff
  };

  function close(){
    screen.hide();
  };

}();

same method applies to other parts of your script (for instance, only create the modal once, then show/hide/resize it, and change image inside).

Style considerations

Just a last minor remark : when declaring many vars, you can do :

var foo = 'foo',
    bar = 'bar',
    baz = 'baz';

It saves typing and it is a bit more clear. In fact, some recommend using the var keyword only once and on top of the scope.

\$\endgroup\$
  • \$\begingroup\$ Outstanding answer. exactly what I was looking for...I will reply with more direct comments and questions once I have had time to study your suggestions further. \$\endgroup\$ – absentx Nov 3 '13 at 23:50
  • \$\begingroup\$ You are completely right about only needing one screen throughout the life cycle and I will be changing the code to reflect that. Would you say it is okay to create the image holder each time though? Reason being it is supposed to be dynamic and take viewport size into account which may change. \$\endgroup\$ – absentx Nov 9 '13 at 4:01

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.