current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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 '14 at 4:06
    
Same code as the $link1, except now the 'active' is being applied to $link2, etc –  RENE VILLA Feb 28 '14 at 4:11

1 Answer 1

up vote 3 down vote

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 '14 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.