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)