HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Removeclass addclass multiple selectors optimization

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
addclassselectorsremoveclassoptimizationmultiple

Problem

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
  });

}

Solution

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

  });

Code Snippets

$('.general').click(function() {
    $('.general').css({border: '5px solid red'});
});
$('.general').click(function() {
    $(this).css({border: '5px solid red'});
});
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

  });

Context

StackExchange Code Review Q#43010, answer score: 4

Revisions (0)

No revisions yet.