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've written a function that works fine, but being new to jQuery, I'd like some reviews on writing this more cleanly. Anything advice would help; just trying to learn!

function displayContent() {
  var $link1 = $('.row.nav li a.bio');
  var $link2 = $('.row.nav li a.stylist');
  var $link3 = $('.row.nav li a.contact');
  var $content = $('#text-content')
  var $bio = $("#bio");
  var $stylist = $("#stylist");
  var $contact = $("#contact");
  var $overlay = $('.content-overlay');

  //link1
  $link1.click(function (e) {
    e.stopPropagation();
    $link2.removeClass('active');
    $link3.removeClass('active');
    $link1.addClass('active');
    $contact.hide();
    $stylist.hide();
    $bio.fadeIn(700);
    $overlay.show();
  });
  //link2
  $link2.click(function (e) {
   //same code here
  });
  //link3
  $link3.click(function (e) {
    //same code here
  });


  //close overlay/hide content
  $('html').click(function (e) {
//hide code
  });

}
share|improve this question
    
When you say // same code here does that mean that both of those handlers are identical? –  cookie monster Feb 28 at 4:06
    
Same code as the $link1, except now the 'active' is being applied to $link2, etc –  RENE VILLA Feb 28 at 4:11

2 Answers 2

up vote 2 down vote accepted
function displayContent() {
   // put all links together
  var $links = $('.row.nav li a').filter(".bio,.stylist,.contact");
  var $content = $('#text-content')
  var $bio = $("#bio");

   // put these together since they're used in the same way
  var $stylist_contact = $("#stylist,#contact");
  var $overlay = $('.content-overlay');

  $links.click(function (e) {
    e.stopPropagation();

     // remove .active from all links
    $links.removeClass('active');

     // add .active to the current link
    $(this).addClass('active');

     // this now hides both at the same time
    $stylist_contact.hide();
    $bio.fadeIn(700);
    $overlay.show();
  });
}
share|improve this answer
    
Perfect! Thanks for the help! –  RENE VILLA Feb 28 at 4:22
1  
Two accepted answers within an hour of joining this site... I'm liking this. ;) –  syb0rg Feb 28 at 4:22
    
got it, thanks. beginners mistake! –  RENE VILLA Feb 28 at 4:24
    
@RENEVILLA It's understandable. Make sure to take the tour for a better understanding of how this site works. You even get a badge for it! –  syb0rg Feb 28 at 4:26
    
awesome, will do! –  RENE VILLA Feb 28 at 4:29

Using $(this) is very helpful in situations where you have similar/same code. The way it works, is if say you have a class .general , which 50 different div's on the page all have that class. But lets say, if a user clicks on any one of those 50 div's , you want ONLY that div that was clicked on to have a border of 5px solid red. So instead of writing

$('.general').click(function() {
    $('.general').css({border: '5px solid red'});
}); 

which would give all 50 div's a border, you can write this

$('.general').click(function() {
    $(this).css({border: '5px solid red'});
}); 

and now only the div that was clicked on will have the border. Also, it will work more than once, so if you then clicked on 10 other div's with class general, they would all get the border red as well.


This can be applied to your code like so : (only included the relevant info)

function displayContent() {

  var $links = $('.row.nav li a.bio, .row.nav li a.stylist, .row.nav li a.contact');

}

  $links.click(function (e) {  // so you just need one click function instead of 3
    e.stopPropagation();
    $links.removeClass('active');  // removes the class active from all $links
    $(this).addClass('active');  // gives the class active to the one clicked on

  });
share|improve this answer
1  
Thanks for the explanation @LowerClassOverflowian! Much appreciated! –  RENE VILLA Feb 28 at 4:25

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.