3
\$\begingroup\$

I have put together a simple Javascript accordion, though the Javascript code I think could possibly be slimmed down. I would welcome any feedback anyone has on what I have done, and any advice to improve.

HTML

<div class="accordion">
  <h4 class="accordion-header">Accordion 1</h4>
  <div class="accordion-content">
    <p>Cras malesuada ultrices augue molestie risus.</p>
  </div>
  <h4 class="accordion-header">Accordion 2</h4>
  <div class="accordion-content">
    <p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Recusandae at placeat nesciunt nostrum accusamus debitis fuga similique quisquam, rerum temporibus, quod asperiores nulla, eveniet libero earum eaque harum inventore minima ipsum saepe omnis. Officia, est, maiores. Reprehenderit odio perspiciatis voluptates commodi ex at praesentium laborum deleniti libero, architecto sit optio repellat est molestiae beatae, magnam qui voluptatibus. Ducimus mollitia dignissimos minus sapiente quidem, animi adipisci laboriosam aliquam asperiores facere. Repellat recusandae doloribus incidunt voluptatibus quibusdam rem delectus inventore nisi, laudantium. Doloribus eum vero, consequuntur nisi enim quam non odio dignissimos praesentium nostrum magnam consequatur totam reprehenderit quaerat. Saepe, blanditiis fugit?</p>
  </div>
  <h4 class="accordion-header">Accordion 3</h4>
  <div class="accordion-content">
    <p>Vivamus facilisisnibh scelerisque laoreet.</p>
  </div>
</div>

SCSS

div.accordion {
  .accordion-content {
    max-height: 0px;
    overflow: hidden;
    transition: max-height 400ms;
    &.active {
      overflow: auto;
    }
  }
}

JavaScript

document.addEventListener('DOMContentLoaded', function() {

  function toggle(){
    if (this.nextElementSibling.classList.contains('active')) {
      this.nextElementSibling.classList.remove('active');
      this.nextElementSibling.removeAttribute('style');
      this.classList.remove('active');
    }
    else {
      var elementHeight = this.nextElementSibling.scrollHeight;
      this.nextElementSibling.classList.add('active');
      this.nextElementSibling.style.maxHeight = elementHeight + 'px';
      this.classList.add('active');
    }
  }

  var accordion = document.querySelector('.js div.accordion');
  var header = accordion.getElementsByClassName('accordion-header');

  for (var i = 0; i < header.length; i++) {
    header[i].addEventListener('click', toggle);
  }

});
\$\endgroup\$
2
\$\begingroup\$

I don't see any issue with code other than below two things.

First, I'll suggest is to use classList.toggle instead of contains and then add/remove class. See browser compability and if target browser does not support toggle method, use polyfill from MDN.

function toggle() {
    if (this.nextElementSibling.classList.contains('active')) {
        this.nextElementSibling.removeAttribute('style');
    } else {
        var elementHeight = this.nextElementSibling.scrollHeight;
        this.nextElementSibling.style.maxHeight = elementHeight + 'px';
    }

    // Toggle `active` class
    this.nextElementSibling.classList.toggle('active');
    this.classList.toggle('active');
}

Second, caching the length of array when iterating using for will slightly improve the performance.

for (var i = 0, len = header.length; i < len; i++) {
              ^^^^^^^^^^^^^^^^^^^^^                  : Caching length

If running the code in latest environment where for...of is supported, you can use it to iterate over elements.

var headers = accordion.getElementsByClassName('accordion-header');
for (header of headers) {
    header.addEventListener('click', toggle);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ The for ... of loop might be an alternative \$\endgroup\$ – le_m May 3 '17 at 10:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.