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.

I'm currently working on my portfolio site and it's all great, but I'm unsure how to DRY my jQuery. I'm repeating myself a lot in the jQuery and it's just messy.

HTML:

 <div id="footer-start" class="row">
      <div class="col-sm-12 col-md-12">

        <ul class="nav nav-pills footer-nav">
          <li><a href="#" class="loadHide" id="clickPortfolio">Portfolio</a></li>
          <li><a href="#" class="loadHide" id="clickAbout">About</a></li>
          <li><a href="#" class="loadHide" id="clickContact">Contact</a></li>
        </ul>

        <span>Click to expand</span>
        <img src="img/arrow.png" alt="" class="loadHide">
      </div>
    </div>

<div id="footer-portfolio" class="row">
      <div class="col-sm-6 col-sm-offset-4 col-md-6 col-md-offset-4">

        <a class="lightbox" href="img/" title=""><img src="img" width="150px" height="150px" /></a>

        <a class="lightbox" href="img/" title="(WIP)"><img src="img/" width="80px" height="150px" /></a>

      </div>
    </div>

<div id="footer-about" class="row">
      <div class="col-sm-8 col-sm-offset-2 col-md-8 col-md-offset-2">
        <h3>About me<small></small></h3>

        <p>Paragraph</p>
      </div>
    </div>

        <div id="footer-contact" class="row">
      <h3>Questions<small></small></h3>

      <div class="col-sm-8 col-sm-offset-2 contact-form">
        <form id="contact" method="post" action="send.php"  class="form" role="form">

          <div class="row">
            <div class="col-xs-6 col-md-6 form-group">
              <input class="form-control" id="name" name="name" placeholder="Name" type="text" required autofocus />
            </div>

            <div class="col-xs-6 col-md-6 form-group">
              <input class="form-control" id="email" name="email" placeholder="Email" type="email" required />
            </div>
          </div> 

          <textarea class="form-control" id="message" name="message" placeholder="Message" rows="7"></textarea>
          <br />

          <div class="row">
            <div class="col-xs-12 col-md-12 form-group">
              <button class="btn btn-primary pull-right" type="submit">Submit</button>
            </div>
          </div>

        </form>
      </div>
    </div>

jQuery:

$(document).ready(function() {




 var $footerPortfolio = $('#footer-portfolio'),
          $footerAbout = $('#footer-about'),
          $footerContact = $('#footer-contact'),
          $footerSent = $('#footer-contact-sent'),
          $clickPortfolio = $('#clickPortfolio'),
          $clickAbout = $('#clickAbout'),
          $clickContact = $('#clickContact'),
          $arrowImg = $('#footer-start img'),
          $arrowTxt = $('#footer-start span'),
          $loadHide = $('.loadHide');
          sentUrl = window.location.href;

      $loadHide.hide();

      $arrowImg.hover(function() {
        $arrowTxt.show('fade', 800);
        },function() {
          $arrowTxt.hide('fade');
        });

      $clickPortfolio.click(function() {
        event.preventDefault();
        $loadHide.slideUp(500);
        $footerPortfolio.slideToggle(500);
      });

      $clickAbout.click(function() {
        event.preventDefault();
        $loadHide.slideUp(500);
        $footerAbout.slideToggle(500);
      });

      $clickContact.click(function() {
        event.preventDefault();
        $loadHide.slideUp(500);
        $footerContact.slideToggle(500);
      });

      if (sentUrl.search("#sent") >= 0) {
        $footerContact.slideUp(500);
        $footerSent.slideDown(800).delay(1000).slideUp(800);
      }
    });

I got a bit cleaner, and would love some feedback on this solution.

 $(document).ready(function() {
  var $footerPortfolio = $('#footer-portfolio'),
      $footerAbout = $('#footer-about'),
      $footerContact = $('#footer-contact'),
      $footerSent = $('#footer-contact-sent');

  var $clickPortfolio = $('#clickPortfolio'),
      $clickAbout = $('#clickAbout'),
      $clickContact = $('#clickContact');

  var $arrowImg = $('#footer-start img'),
      $arrowTxt = $('#footer-start span');

  var $loadHide = $('.loadHide');

  var sentUrl = window.location.href;

  $loadHide.hide();

  $arrowImg.hover(function() {
    $arrowTxt.show('fade', 800);
    },function() {
      $arrowTxt.hide('fade');
    });

 function myClick(obj) {
    event.preventDefault();

    if (obj == $footerPortfolio) {
      $footerAbout.slideUp(500);
      $footerContact.slideUp(500);
      obj.slideToggle(500);
    } else if (obj == $footerAbout) {
      $footerPortfolio.slideUp(500);
      $footerContact.slideUp(500);
      obj.slideToggle(500);
    } else if (obj == $footerContact) {
      $footerPortfolio.slideUp(500);
      $footerAbout.slideUp(500);
      obj.slideToggle(500);
    }
  }

  $clickPortfolio.click(function() {
    myClick($footerPortfolio); }
  );

  $clickAbout.click(function() {
    myClick($footerAbout); }
  );

  $clickContact.click(function() {
    myClick($footerContact); }
  );

  if (sentUrl.search("#sent") >= 0) {
    $footerContact.slideUp(500);
    $footerSent.slideDown(800).delay(1000).slideUp(800);
  }

});

Even better:

 function myClick(obj) {
    event.preventDefault();

    if (obj.css('display') == 'none'){
      $loadHide.slideUp(500);
      obj.slideToggle(500);
    } else if (obj.css('display') != 'none') {
      obj.slideUp(500);
    }
  }
share|improve this question
    
This would be so much easier using a library like Knockout JS. Check it out, you'll never code using jQuery .on selectors again. –  Chris Dixon Jun 6 at 11:46

5 Answers 5

The way you define sentUrl as a global variable is either buggy (if it's unintentional) or deceptive (if it's intentional).

var $footerPortfolio = $('#footer-portfolio'),
      …
      $loadHide = $('.loadHide');
      sentUrl = window.location.href;

Since the difference between a local and a global variable hinges on the distinction between a comma and semicolon, I prefer to see such multi-line definitions to be written as

var $footerPortfolio = $('#footer-portfolio');
var $footerAbout = $('#footer-about');
var $footerContact = $('#footer-contact');
var $footerSent = $('#footer-contact-sent');
var $clickPortfolio = $('#clickPortfolio');
var $clickAbout = $('#clickAbout');
var $clickContact = $('#clickContact');
var $arrowImg = $('#footer-start img');
var $arrowTxt = $('#footer-start span');
var $loadHide = $('.loadHide');
var sentUrl = window.location.href;

If it is intentionally a global variable, then fix your indentation and add vertical spacing so that

sentUrl = window.location.href;

stands alone.

share|improve this answer
    
It makes a lot of sense and didn't realize it myself, thanks! –  user44754 Jun 5 at 19:16

Be Consistent

You create two different styles of Image tags,

  • self closing

    <img src="img/" width="80px" height="150px" />
    
  • not closed

    <img src="img/arrow.png" alt="" class="loadHide"> 
    

In HTML5 it doesn't matter which way you do this as both will work, but you should stay consistent, personally I use the self closing image tags.


Reduce Clutter

I noticed that you do this a couple of times as well

<h3>About me<small></small></h3>

why do you have the <small> tag in there with nothing in it? if you don't need it there don't use it, extra tags add to clutter.


Use Standard Formatting

Use proper indentation and Newline characters throughout your HTML

<a class="lightbox" href="img/" title=""><img src="img" width="150px" height="150px" /></a>

should look like this

<a class="lightbox" href="img/" title="">
    <img src="img" width="150px" height="150px" />
</a>

it makes it easier for the next person to read and travel through your HTML

share|improve this answer
1  
Thanks for pointing this out, completely missed it. –  user44754 Jun 5 at 19:18
    
you are welcome. All of these things make it easier to maintain as well. –  Malachi Jun 5 at 19:24

You can move all of the click functionality into one function:

function myClick(obj)
{
    event.preventDefault();
    $loadHide.slideUp(500);
    obj.slideToggle(500);
}

Then call that function inside each of your click mappings:

$clickPortfolio.click(function() {myClick($footerPortfolio)});

$clickAbout.click(function() {myClick($footerAbout)});

$clickContact.click(function() {myClick($footerContact)});
share|improve this answer
    
Awesome, thank you! This is exactly what I was looking for but didn't know. I'll be using this for reference in the future. –  user44754 Jun 5 at 19:46
    
The issue I'm having now is the page loads, slides it all up and the last element stays displayed. In this case '$footerContact'. –  user44754 Jun 5 at 20:07
    
I think the bug was occurring because the myClick() code was actually getting called. I have updated my answer. All that should be needed is to wrap that call in an anonymous function. –  BeetDemGuise Jun 5 at 20:11
    
Awesome, thanks. I just realized I made a mistake in the HTML with what I want to hide. I actually want to hide the "footer-portfolio", "footer-about" and "footer-contact" divs and toggle them via the buttons. With the code you've updated it does that but the '$loadHide.slideUp' followed by the toggle is making it have a non-stop cycle of sliding down. Any suggestions? –  user44754 Jun 5 at 20:18
    
I've done the following but wondering if there is a cleaner way. function myClick(obj) { event.preventDefault(); if ($clickPortfolio) { $footerAbout.slideUp(500); $footerContact.slideUp(500); obj.slideToggle(500); } } –  user44754 Jun 5 at 20:33

Part 1: Introduce a "data-myapp-target" attribute to create a mapping between the links and the footers. The "myapp" portion is just to avoid collision with anything else that might be using a similarly named "data-" attribute; use whatever value is appropriate for your application.

      <li><a href="#" class="loadHide" id="clickPortfolio" data-myapp-target="footer-portfolio">Portfolio</a></li>
      <li><a href="#" class="loadHide" id="clickAbout" data-myapp-target="footer-about">About</a></li>
      <li><a href="#" class="loadHide" id="clickContact" data-myapp-target="footer-contact">Contact</a></li>

Part 2: Use that attribute in your jQuery code:

  $loadHide.click(function() {
    event.preventDefault();
    $loadHide.slideUp(500);
    $("#" + $(this).data("myapp-target")).slideToggle(500);
  });

This code assumes that the data-myapp-target attribute will correspond to the id of another element.

share|improve this answer
    
This is a great idea. It's exactly what data- attributes are for. –  WChargin Jun 6 at 4:47
1  
This idea isn't bad, but the code should probably read $(this).data("myapp-target") instead. –  Schism Jun 6 at 13:22
    
@Schism Thanks for that suggestion! Answer has been updated. –  Dr. Wily's Apprentice Jun 6 at 14:10

From your last update:

function myClick(obj) {
    event.preventDefault();

    if (obj.css('display') == 'none'){
      $loadHide.slideUp(500);
      obj.slideToggle(500);
    } else if (obj.css('display') != 'none') {
      obj.slideUp(500);
    }
}

You don't need the nested if, as the object or is visible or is not. Moreover, you can pass an extra parameter that you default to 500 for the duration of the animation.

function myClick(obj, duration) {
    event.preventDefault();

    // Defaults an optional parameter for the duration of the animation.
    duration = duration || 500;

    if (obj.is(":visible")){
      $loadHide.slideUp(delay);
    }
    obj.slideToggle(delay);
}
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.