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.

Does this compile? You should include all code to ensure it's on topic. — Raystafarian
– Raystafarian, Commented Feb 9, 2016 at 14:48
@Raystafarian I could. But I felt the question would get bloated. There's a lot of code. I just thought all the onClick events look ugly and when trying to go through my code and improve it I had no idea how I would tidy up multiple onClick events — pourmesomecode
– pourmesomecode, Commented Feb 9, 2016 at 15:17
Fair enough, did you see codereview.stackexchange.com/questions/26229/… — Raystafarian
– Raystafarian, Commented Feb 9, 2016 at 15:54
Is it the only part of your code regarding these onClick events? If yes, I don't see any way to make it more readable: factorization is possible only on a few element/actions, and will have a cost on readability. At the opposite, if this ony an example, while a lot of other onClick events exist, then it might be considered. In this case, post the entire code. — cFreed
– cFreed, Commented Feb 10, 2016 at 0:33
Review code organization concept here learn.jquery.com/code-organization/concepts. You can create separate object for encapsulation show/view logic. For example, create NavWrapper. — Viktor
– Viktor, Commented Feb 10, 2016 at 16:26

Roamer-1888 · Accepted Answer · 2016-02-12 14:01:33Z

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

Gary Storey · Accepted Answer · 2016-02-10 18:26:39Z

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.

200_success · Accepted Answer · 2016-02-10 19:43:42Z

0

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.

answered Feb 10, 2016 at 19:43

200_success

146k22 gold badges191 silver badges481 bronze badges

Add a comment |

Stack Exchange Network

Multiple jQuery onClick events

3 Answers 3

You must log in to answer this question.

Linked

Hot Network Questions

Multiple jQuery onClick events

3 Answers 3

You must log in to answer this question.

Linked

Related

Hot Network Questions