Multiple jQuery statements in one function

Question

I've been working with jQuery for a few days now and starting to get the hang of it (it does the thing that I want to do).

I've made an accordion, and the header of each accordion item has an arrow that points up (for being open) or down (for being closed). This code works fine, does what it has to do. When I wrote it, it felt very ugly.

When I press a header of an item in the accordion, function gets called like this:

<div class="accordionHeadingDiv" onclick="leftActive(0, this)">

The function itself:

function leftActive(e, b) {
  $('.active').removeClass('active');
  $('.relative').eq(e).addClass('active');

  $('.arrowUp').addClass('arrowDown');
  $('.arrowUp').removeClass('arrowUp');
  $('.arrowDown', b).addClass('arrowUp');
  $('.arrowDown', b).removeClass('arrowDown');
}

HTML structure:

  <div class="overzicht">
    <div id="accordion">
      <div class="accordionHeadingDiv" onclick="leftActive(0, this)">
        <h3 class="accordionHeading"> Transport logistics </h3>
        <div class="arrowUp">
        </div>
      </div>
      <div class="accordionContent">

      <?php require 'templates/dummyBedrijven.php'; ?>

      </div>
      <div class="accordionHeadingDiv" onclick="leftActive(1, this)">
        <h3 class="accordionHeading"> Real Estate </h3>
        <div class="arrowDown">
        </div>
      </div>
      <div class="accordionContent">

      <?php require 'templates/dummyBedrijven.php'; ?>

      </div>
      <div class="accordionHeadingDiv" onclick="leftActive(2, this)">
        <h3 class="accordionHeading"> Salt trade Winter Maintenance </h3>
        <div class="arrowDown">
        </div>
      </div>
      <div class="accordionContent">

      <?php require 'templates/dummyBedrijven.php'; ?>

      </div>
      <div class="accordionHeadingDiv" onclick="leftActive(3, this)">
        <h3 class="accordionHeading"> Human Resources Solutions </h3>
        <div class="arrowDown">
        </div>
      </div>
      <div class="accordionContent">

      <?php require 'templates/dummyBedrijven.php'; ?>

      </div>
    </div>
  </div>

Answer 1

First off, don't use the onclick attribute. You have jQuery, so use it to set the appropriate event handler:

$(".accordionHeadingDiv").click(function (event) {
  ...
});

That will add a click-event handler to all the headings.

Now, as far as I can tell, your code then removes the active class from all other accordion elements, adds it to the one that was clicked, and does the arrow-replacement.

This is simpler with the event handler setup above:

$(".accordionHeadingDiv").click(function (event) {
  $("#accordion .active").removeClass("active"); // remove "active" from others
  $(this).addClass("active"); // add it to the heading that was clicked
});

For the arrows, I'd advice a better class name system and more CSS. Call the class simply "arrow", and don't bother changing it via JS. Since you set the heading to active all you need is CSS like this

.accordionHeadingDiv .arrow {
  /* default style (i.e. arrow pointing down) */
}

.accordionHeadingDiv.active .arrow {
  /* active style (i.e. arrow pointing up) */
}

No need for JS.

While I'm at it, I'd advice you to make better use of semantic HTML and CSS:

1. Don't call a class accordionHeadingDiv. The class should be independent of the element type. If you want to style based on element and class, the basic CSS syntax of div.accordionHeading is better.
2. Actually, don't call it accordionHeading either. It's nested inside the #accordion element, so you don't need to prefix the class name. You can style it with just #accordion .heading
3. Actually, don't even use heading: Use a heading element! You've already got one, but there's no need for the DIV around it.

What you get is something like

<div id="accordion">
  <h3>
    Transport logistics
    <span class="arrow"></span>
  </h3>
  <div class="content">
    <?php require 'templates/dummyBedrijven.php'; ?>
  </div>

  ...

</div>

Much cleaner. (P.s. I'd use include instead of require)

However...

That's all well and good, but if I understand you correctly, jQueryUI is handling all the accordion stuff. And jQueryUI adds its own "active" class to the opened accordion-element. So you can actually get rid of all you custom JS and just stick to CSS:

#accordion .ui-accordion-header .arrow {
  /* default style (i.e. arrow pointing down) */
}

#accordion .ui-accordion-header-active .arrow {
  /* active style (i.e. arrow pointing up) */
}

All you need in the JS is:

$('#accordion').accordion();

Here's a demo