current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Please review my first query project, it achieves the desired result, but am I doing it right?

window.onresize = centreBoxInViewport;

function displayLightBox(obj) {

    // Get the path to the image
    var path = obj.getAttribute("href");

    // Create a new image, and set its source.
    var image = new Image();

    // The onload function must come before we set the image's src, see link for explanation.
    // http://www.thefutureoftheweb.com/blog/image-onload-isnt-being-called.

    // Anonymous function set to onload event, to make sure we only proceed if the image has been loaded.
    image.onload = function() {
        $('#image').attr("src", path)
        centreBoxInViewport();
        showOverlay();

        // Bind the hide functions here, as the elements have now been loaded into the DOM.
        $('#overlay').click(hideLightBox);
    };

    // Set the src, and show the image.
    image.src = path;
}

function hideLightBox() {
    fadeBox();
}

function showBox(){
    $('#lightbox').fadeTo(200, 1);
}

function fadeBox(){
    $('#lightbox').fadeTo(200, 0, function() {
        $('#lightbox').hide();
        hideOverlay();
    });
}

function centreBoxInViewport() {
    // Get the dimensions of the viewport.
    height = $(window).height()
    width = $(window).width();

    // Position the box.
    $('#lightbox').css('top', (height/2) - parseInt($('#lightbox').height())/2);
    $('#lightbox').css('left', (width/2) - parseInt($('#lightbox').width())/2);

}

function showOverlay() {
    $('#overlay').fadeTo(200, 0.5, function() { showBox() });
}

function hideOverlay() {
    $('#overlay').fadeTo(200, 0, function() {
        $('#overlay').hide();
    });
}

HTML

<div id="overlay" style="display: none"></div>

<div id="lightbox" style="display:none">
    <div>
        <a href="off somewhere" id="close" onclick="hideLightBox();return false;"></a>
        <img id="image" alt="" src="">
    </div>
</div>

CSS

div#overlay { position: fixed; background-color:#000000; top: 0px; left: 0px; width: 100%; height: 100%; z-index: 99; }
div#lightbox { background-color: white; border: 5px solid white; z-index: 100; position: fixed; }

Thanks

share|improve this question

migrated from stackoverflow.com Feb 21 '12 at 20:05

This question came from our site for professional and enthusiast programmers.

2 Answers 2

up vote 4 down vote accepted

Looks fine to me, but I do have a few recommendations:

Instead of calling the onclick handler inline, consider doing the following:

$('#close').click(function(e) { fadeBox(e); });

Then I would pass e to fadeBox() and do e.preventDefault() so that the click event doesn't throw the user to the top of the page.

Also, you do not need to create the function hideLightBox() if the only thing it is doing is calling fadeBox().

share|improve this answer
    
Thanks for your help Nick! –  pingu Feb 21 '12 at 22:13
up vote 1 down vote

Can't you use .prepend or .append to add the image element and save some code in displayLightBox()?

$('#lightbox div').append('<img id="image" src="' + path + '" />');

Declare your height and width in centreBoxInViewport().

Declare your top and left as two new variables and then combine the .css():

var boxtop = (height / 2) - $('#lightbox').height() / 2;
var boxleft = (width / 2) - $('#lightbox').width() / 2;
$('#lightbox').css({top: boxtop, left: boxleft});

I don't think you need the parseInt() because height() and width() should always return numeric.

I second Nick's suggestion that you don't need hideLightBox() if you're just calling fadeBox().

You should try to be consistent with function naming-- use "display" or "show" but not both.

Remove the inline styles from your HTML and put in your CSS. Remove the onclick from your HTML and put it in your JS:

$('#close').click(function() { hideLightBox(); return false; });
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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