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)

        // Bind the hide functions here, as the elements have now been loaded into the DOM.

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

function hideLightBox() {

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

function fadeBox(){
    $('#lightbox').fadeTo(200, 0, function() {

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() {


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

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


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; }
  • \$\begingroup\$ Your #lightbox has style display: none, so there is nothing visible to click on. \$\endgroup\$ – 200_success Nov 30 '15 at 6:31

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().

| improve this answer | |

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; });
| improve this answer | |

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.