Multiple jQuery onClick events

Question

I have a ton of jQuery onClick events. onClick I hide/show different UI elements. I was wondering, how can I tidy the code up and make multiple onClick events more readable?

$('.info_2').on('click', function() {
  $('#nav-wrapper').toggleClass('hidden_nav');
  $('#card-wrapper').toggleClass('centre_share');
  $('.E_info').toggleClass('display');
  $('#info-btn').css('opacity', '0');
  $('#nav-wrapper').delay(300).toggleClass('hidden');
  $('#nav-wrapper').removeClass('display_nav');
  $('#nav-wrapper').removeClass('display');
});

$('.info_back').on('click', function() {
  $('#nav-wrapper').removeClass('hidden_nav');
  $('#nav-wrapper').addClass('display_nav');
  $('#nav-wrapper').addClass('display');
  $('#info-btn').css('opacity', '1');
  $('#nav-wrapper').removeClass('hidden');
  $('.E_info').removeClass('display');
  $('.E_info').addClass('hidden');
  $('#card-wrapper').removeClass('centre_share');
});

$('#info-btn').on('click', function(){
  $('#info-btn').toggleClass('close_btn');
  $('.o-card_border').toggleClass('info_display card_active');
  $('.start_title').toggleClass('hidden remove_flow')
  $('#svg_full').attr('class', 'test');
  $('#svg_top').attr('class', 'test');
  $('#svg_bot').attr('class', 'test');
  $('#svg_bot_bot').attr('class', 'test');
  $('#svg_bot_right').attr('class', 'test');
  $('.rectangle_style_frame3 display').toggleClass('hidden');
  $('.triangle_style').toggleClass('hidden');

  $('.bg-info').toggleClass('display');
  $('.info_CharactersInvolved').toggleClass('display');
  $('.info_themes').toggleClass('display');
  $('.E_info').toggleClass('display');
});

The code works fine. I just think it looks really ugly. And the readability of it is painful, especially if you're trying to jump onto the project and learn the codebase.

Answer 1

The worst issues are cases of the same element being selected multiple times in order to apply various methods. Repeat selection of the same element should be avoided because selection of DOM element(s) is expensive.

There are also are cases where multiple selections have the same method applied. This isn't so bad for performance, but leads to unnecessarily bulky source.

The code can be improved with the following techniques :

• using method chaining.
• using comma separated selectors.
• using $(this) to select the same element as the one to which an event handler is attached.
• passing space-separated lists of class names to addClass() and .removeClass().

You might end up with something like this :

$('.info_2').on('click', function() {
    $('#card-wrapper').toggleClass('centre_share');
    $('.E_info').toggleClass('display');
    $('#info-btn').css('opacity', '0');
    $('#nav-wrapper').toggleClass('hidden_nav').delay(300).toggleClass('hidden').removeClass('display_nav').removeClass('display');
});

$('.info_back').on('click', function() {
    $('#nav-wrapper').removeClass('hidden_nav hidden').addClass('display_nav display');
    $('#info-btn').css('opacity', '1');
    $('.E_info').removeClass('display').addClass('hidden');
    $('#card-wrapper').removeClass('centre_share');
});

$('#info-btn').on('click', function(){
    $(this).toggleClass('close_btn');
    $('.o-card_border').toggleClass('info_display card_active');
    $('.start_title').toggleClass('hidden remove_flow');
    $('#svg_full, #svg_top, #svg_bot, #svg_bot_bot, #svg_bot_right').attr('class', 'test');
    $('.rectangle_style_frame3 display, .triangle_style').toggleClass('hidden');
    $('.bg-info, .info_CharactersInvolved, .info_themes, .E_info').toggleClass('display');
});

Answer 2

First, you should wrap your code up in a closure to prevent collisions with other code. I typically use an IIFE for this. Also, you should add use strict'; to your code to help prevent certain issues.

(function($) {
   'use strict';
   // your code here
})( jQuery ); 

You can abstract away all of those class names so that, if you ever have to make a change, you only have to change them in place.

var classes = {
  hideNav : 'hidden-nav',
  hide : 'hiddden'
};

//example of how to use
$('#nav-wrapper').toggleClass( classes.hide );

I would also recommend not using the display classes. Just removing the hidden class should be sufficient. If you additional CSS in the display class, then you should create a class called nav (or whatever make sense) for styling.

Next, you need to DRY your code. In your functions, you are repeating a LOT of code. These repeated calls should be their own function. For example, you do the same manipulations on the .nav-wrapper in each set of code. Move that code into it's own function and just call it where ever you need.

Third, you should cache all of your selectors. As a general rule of thumb, if you use the same selection more than once, you should cache it. So create a variable and use the variable everywhere.

var $navwrapper = $('#nav-wrapper');
$navwrapper.removeClass( classes.hide );

Last, although you haven't supplied any HTML, there are probably ways you might be able to simplify your code. For instance, if all of your .info* items have the same parent container (ex: .info-areas), you could add the click event to this parent element instead of each individual info area. You can figure out which area was clicked by using event.target.

$('.info-areas').on('click', function(evt) {
  var current=$(evt.target);
  // do whatever
});

Hope that helps.

Answer 3

Your question is basically equivalent to this one, and my advice is the same. Each click handler is really changing global state, so set the state once on some ancestor element (e.g. the <body>) and let the Cascading stylesheet take care of all the consequences.