Can I improve my jquery dialog code, critisicm welcome

Question

Fairly new to jquery, can i improve this code please?

HTML

<a href="#" class="dialog" id="login">login</a>

Javascript

function Dialog() {

    // Event handler for a dialog click.
    $('a.dialog').click(function () {

        var dialog = $("#" + this.id + "_dialog");

        // Bind the hide functions here, as the elements have now been loaded into the DOM.
        $("#overlay, #close").click(function () {
            HideDialog(dialog)
        });
        window.onresize = function () {
            CentreBoxInViewport(dialog);
        };

        SetupDialog(dialog);
        return false;
    });

    // Event handler for an image click.
    $('div.status > a').click(function () {

        // Get the object for the lightbox.
        var dialog = $("#lightbox");

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

        // Bind the hide functions here, as the elements have now been loaded into the DOM.
        $("#overlay, #close").click(function () {
            HideDialog(dialog)
        });
        window.onresize = function () {
            CentreBoxInViewport(dialog);
        };

        SetupLightBox(path, dialog);
        return false;
    });
}

function SetupLightBox(path, dialog) {

    // Create a new image.
    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(dialog);
        ShowOverlay(dialog);
    };

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

function SetupDialog(dialog) {
    CentreBoxInViewport(dialog);
    ShowOverlay(dialog);
}

function ShowDialog(dialog) {
    dialog.fadeTo(200, 1);
}

function HideDialog(dialog) {
    dialog.fadeTo(200, 0, function () {
        dialog.hide();
        HideOverlay();
    });
}

function ShowOverlay(dialog) {
    $('#overlay').fadeTo(200, 0.5, function () {
        ShowDialog(dialog)
    });
}

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

function CentreBoxInViewport(dialog) {

    // Get the dimensions of the viewport.
    var height = $(window).height()
    var width = $(window).width();

    // Calculate the position of the lightbox.
    var boxtop = (height / 2) - dialog.height() / 2;
    var boxleft = (width / 2) - dialog.width() / 2;

    // Position the box.
    dialog.css({
        top: boxtop,
        left: boxleft
    });
}

Answer 1

• In JavaScript it is custom to give functions names that start with a lowercase letter so that they are not confused with objects.

• Personally I'd avoid "hardcoding" the connection of the id of the link with the id of the dialog. (login -> login_dialog). Instead you could use the href of the link to refer to dialog. Example:

  <a href="#login_dialog" class="dialog">login</a>

  var dialog = $(this.href.replace(/.*(?=#[^\s]*$)/, ''));

(The replace is necessary due to bugs in IE < 8)

• You are repeatedly assigning the click and resize event handler on each click, so that each time a dialog is shown a additional handler is added, but never removed.

(There is more, however I don't have time right now. Maybe I'll come back later)