Is there a more efficient way to toggle multiple classes on click than this?

Question

What is a better way to implement this toggle class using JS?

What this is doing is simply this:

When you click either "Services", "Gallery", or "Customer", it removes the class "hidden" in the div below, and adds the class "hidden" to all other lists within the same div.

A live demo of what is happening can be seen here: http://www.cyberbytesdesign.com/WIP2 (it's the main header navigation).

Here is the code that I am using (definitely not an efficient way as this is less than half of all the code, but gets the point across).

<nav>
  <ul class="menu-option-set">
    <li>
      <a href="javascript:;" onmouseup="
        document.getElementById('services').classList.toggle('hidden');
        document.getElementById('gallery').classList.add('hidden');
        document.getElementById('customer').classList.add('hidden');
        ">Services</a>
    </li>
    <li>
       <a href="javascript:;" onmouseup="
         document.getElementById('gallery').classList.toggle('hidden');
         document.getElementById('services').classList.add('hidden');
         document.getElementById('customer').classList.add('hidden'); ">Gallery</a>
    </li>
    <li>
       <a href="javascript:;" onmouseup="
         document.getElementById('gallery').classList.toggle('hidden');
         document.getElementById('services').classList.add('hidden');
         document.getElementById('customer').classList.add('hidden'); ">Customer</a>
    </li>
  </ul>
</nav>
<div id="header-subnav">
  <nav>
    <ul id="services" class="hidden">
      <li <?php echo $bathroom ?>><a href="bathroom">Bathroom</a></li>
      <li <?php echo $kitchen ?>><a href="index.php">Kitchen</a></li>
      <li <?php echo $accessibility ?>><a href="index.php">Accessibility</a></li>
    </ul>
    <ul id="gallery" class="hidden">
      <li <?php echo $photo ?>><a href="gallery.php">Photo Gallery</a></li>
      <li <?php echo $project ?>><a href="project.php">Project Gallery</a></li>
    </ul>
    <ul id="customer" class="hidden">
      <li <?php echo $coupons ?>><a href="coupons.php">Coupons</a></li>
      <li <?php echo $testimonials ?>><a href="testimonials.php">Testimonials</a></li>
    </ul>
  <nav>
</div>

I'm assuming that you have to do something similar to using this somehow, but modified:

$('.menu-option-set a').click(function()
{
    // if clicked item is selected then deselect it
    if ($('#header-subnav').hasClass('hidden'))
    {
        $('#header-subnav').removeClass('hidden');
    }

    // otherwise deselect all and select just this one
    else
    {
        $('.menu-option-set a').removeClass('hidden');
        $('#header-subnav').addClass('hidden');
    }
});

Any ideas?

Mics · Accepted Answer · 2012-07-24T10:48:25.133

2

Less modification version:

Add data attribute to main menu and set its value to sub menu's id.

<nav>
  <ul class="menu-option-set">
    <li>
      <a href="#" data-subid="services">Services</a>
    </li>
    <li>
       <a href="#" data-subid="gallery">Gallery</a>
    </li>
    <li>
       <a href="#" data-subid="customer">Customer</a>
    </li>
  </ul>
</nav>

Then attach event handler to main menu. It hides all sub menus, and shows proper sub menu. (Also you don't need 'hidden' class.)

$(function() {
    $("#header-subnav ul").hide();
    $('.menu-option-set a').on('click', function() {
        var $s = $("#" + $(this).attr('data-subid'));                
        if($s.is(':hidden')) {
            $("#header-subnav ul").hide();
            $s.show();
        } else {
            $s.hide();
        }
    });
});

That's all! Here's example: http://jsfiddle.net/dT225/1/

...But I suggest this style personally:

Put sub menus next to each main menu.

<nav id="menu">
  <ul>
    <li>
      <a href="#">Services</a>
      <ul>
        <li <?php echo $bathroom ?>><a href="bathroom">Bathroom</a></li>
        <li <?php echo $kitchen ?>><a href="index.php">Kitchen</a></li>
        <li <?php echo $accessibility ?>><a href="index.php">Accessibility</a></li>
      </ul>
    </li>
    <li>
      <a href="#">Gallery</a>
      <ul>
        <li <?php echo $photo ?>><a href="gallery.php">Photo Gallery</a></li>
        <li <?php echo $project ?>><a href="project.php">Project Gallery</a></li>
      </ul>
    </li>
    <li>
      <a href="#">Customer</a>
      <ul>
        <li <?php echo $coupons ?>><a href="coupons.php">Coupons</a></li>
        <li <?php echo $testimonials ?>><a href="testimonials.php">Testimonials</a></li>
      </ul>
    </li>
  </ul>
</nav>

When main menu clicked, event handler will show just next element of source element.

$(function() {
    $('#menu li ul').hide();
    $('#menu a').on('click', function() {
        var $s = $(this).next();           
        if($s.is(':hidden')) {
            $('#menu li ul').hide();
            $s.show();
        } else {
            $s.hide();
        }
    });
});

It's more simple, maintainable, and semantic I think. http://jsfiddle.net/dKtVK/1/

edited Jul 24 '12 at 10:48

answered Jul 24 '12 at 10:32

Mics

1,420
14
19

For the way that I have the site set up, and with the knowledge that I currently have, I don't know how to make a nice JS nav which would allow for the second method, but have it be a different div under it like it currently is. I will definitely try out the first method, supplied JSfiddle for both, you're awesome! Thanks so much! :) – efficient Jul 24 '12 at 10:35
Holy crap, that was quite useful! Thanks so much! There is one issue though, I had it toggle before so you could click the same button twice and it would initially show it, then hide it the second click, etc. Is that possible with this method? I'm assuming a simple line of code will fix this? (using the first method) – efficient Jul 24 '12 at 10:43
Oh, I missed it. I just updated my answer and jsfiddle example. Check it out! – Mics Jul 24 '12 at 10:48
Thank you SOOOOOO Much! You're awesome!!!! Wow, you just saved me so much time.. I have spent ages upon ages with that code that I wrote up and it takes so long to get it working too, sooo messy. I have one question, you put in which is fine, but when you are scrolled down at all and click it, it bumps you up to the top of the page. Is it alright to just throw in "" instead, as it functions nicely and doesn't seem to be problematic? Or is there a better way of doing this/should I not do it this way? Thanks again, you have just helped me out SO))) much! – efficient Jul 24 '12 at 11:02
`javascript:` is bad habit, see [this](http://stackoverflow.com/questions/2479557/why-is-it-bad-practice-to-use-links-with-the-javascript-protocol) article. And of course, you can stop scrolling up by [`preventDefault()`](http://api.jquery.com/event.preventDefault). – Mics Jul 24 '12 at 11:46
Hey, thanks a lot (again)! I put it directly after this: $('.menu-option-set a').click(function() { event.preventDefault(); It seems to be working fine there, is that the correct placement though? Thanks for everything, it has turned out great! – efficient Jul 25 '12 at 01:20
EDIT: It killed my "home" button now though as I have the home button actually link to a page, how do I remedy this now though? – efficient Jul 25 '12 at 01:52
1) That's correct place, but you should pass event as parameter. 2) Update `'.menu-option-set a'` to `'.menu-option-set a[data-subid]'`. [Here's complete solution.](http://jsfiddle.net/KrpFn) – Mics Jul 25 '12 at 03:56
Hey, thanks so much again! I honestly appreciate everything. I definitely understand why it works how it does now, thanks! – efficient Jul 25 '12 at 11:29

Christoph · Answer 2 · 2012-07-24T10:13:54.680

First of all thumbs up for trying to do this with plain javascript. Note however, that using things like classList is not supported in every browser - your code will fail in IE.

To your question: You almost answered your question by yourself. It is strongly advised to NOT use those event attributes an the elements. Instead put it into (a separated) js (file). This makes your code clean and maintainable.

So write this instead:

<a class="services" href="javascript:;">Services</a>

and then add an eventhandler in your <script> Tag or separate JS-File:

var gallery  = document.getElementById('gallery');
var services = document.getElementById('services');
var customer = document.getElementById('customer');

document.querySelector(".services").addEventListener(function(){
    /* Do your stuff here */
    gallery.classList.toggle('hidden');
    /*...and so on...*/
});

I used querySelector here, because it is quite similar to jQuery, thus familiar to lot of people, and supported in IE >=8. But feel free to use whatever suits you (and is needed for browser-support).

Using jQuery (the second codesnippet you posted) makes this quite a lot easier and is working fine in all browsers, so feel free to use it and save you some work.

About your classList comment about it failing in IE, don't worry, I'm one step ahead of ya (http://www.cyberbytesdesign.com/WIP2/resource/js/classList.min.js) is being called in if it's IE (; --> you can check it out for yourself if you want :P. I know the way I have is terrible, I'm gonna try your method out and see if I have any luck! Thanks for the quick response! :) — efficient, Jul 24 '12 at 10:12
EDIT: You mentioned that I could use my second code snippet to save myself some work, but do you have any idea what I'm doing wrong with it? It's not working out so well the way I have it typed out. Thanks again! — efficient, Jul 24 '12 at 10:19

Is there a more efficient way to toggle multiple classes on click than this?

2 Answers2

Less modification version:

...But I suggest this style personally: