views:

305

answers:

4

The code below is for a simple newsletter signup widget.

I'm sure there's a way to make it more concise, any ideas?

var email_form = $('.widget_subscribe form'); 
var email_submit = $('.widget_subscribe .submit');
var email_link = $('.widget_subscribe .email');

// Hide the email entry form when the page loads
email_form.hide();

// Show the form when the email link is clicked
$(email_link).click( function () {
 $(this).toggle();
 $(email_form).toggle();
 return false;
});

// Hide the form when the form submit is clicked
$(email_submit).click( function () {
 $(email_link).toggle();
 $(email_form).toggle();
});

// Clear/reset the email input on focus 
$('input[name="email"]').focus( function () {
    $(this).val("");
    }).blur( function () {
     if ($(this).val() == "") {
      $(this).val($(this)[0].defaultValue);
     }
});
+3  A: 

It's already pretty concise, there's not much more you can do.

Anywhere you have $(email_submit) you can just have email_submit, because you've already wrapped it in $() (which makes it a jquery object).

Eg:

email_submit.click( function () {
    email_link.toggle();
    email_form.toggle();
});
gregmac
+11  A: 

You have some similar code here.

// Show the form when the email link is clicked
$(email_link).click( function () {
        $(this).toggle();
        $(email_form).toggle();
        return false;
});

// Hide the form when the form submit is clicked
$(email_submit).click( function () {
        $(email_link).toggle();
        $(email_form).toggle();
});

It could be refactored so the similarity is obvious.

// Show the form when the email link is clicked
$(email_link).click( function () {
        $(email_link).toggle();
        $(email_form).toggle();
        return false;
});

// Hide the form when the form submit is clicked
$(email_submit).click( function () {
        $(email_link).toggle();
        $(email_form).toggle();
});

So you could wrap toggling the link and the form into a function.

var toggleEmailLinkAndForm = function () {
        $(email_link).toggle();
        $(email_form).toggle();
}   

$(email_link).click(toggleEmailLinkAndForm);
$(email_submit).click(toggleEmailLinkAndForm);

And as others have pointed out, you can drop the redunant $()s.

var toggleEmailLinkAndForm = function () {
        email_link.toggle();
        email_form.toggle();
}   

email_link.click(toggleEmailLinkAndForm);
email_submit.click(toggleEmailLinkAndForm);
Patrick McElhaney
+1  A: 

Like Patrick said (+1), and you can also skip the extra function:

email_submit.click(function () {
    email_link.toggle();
    email_form.toggle();
});
email_link.click(function () {
    email_submit.click(); //calls the click function already subscribed
    return false;
});
Keith
+2  A: 

I like Patrick McElhaney Code Best.

toggleEmailLinkAndForm() {
    email_link.toggle();
    email_form.toggle();
}   

email_link.click(toggleEmailLinkAndForm);
email_submit.click(toggleEmailLinkAndForm);

Part of refactoring is not going overboard. I would not recommend creating an extra click event that calls the other click event. The point of refactoring is readability and flexibility. You can also use the jQuery method "add" to shrink the code but it will become even harder to read.

email_link.add(email_submit).click(function(){
    email_link.add(email_form).toggle();
});
gradbot