Vanilla JS/CSS -- set of buttons to trigger sliders by transitioning height value

Question

I've been working on a solution that allows a set of buttons to reveal divs that slide down from above on-click. I needed each button to toggle an active state for itself, but also if the user were to click on one button, reveal a div, and then go on to click another button, that the initially revealed div transition back up and dissapear while the new one appears.

When all the buttons are on one line, my solution works relatively well. But when there are buttons below some of the hidden divs, suddenly the divs those buttons trigger appear from below instead of dropping down from above... IF one of the buttons above has an active state... If not, business as usual.

Appologies if this is confusing, please see my fiddle : https://jsfiddle.net/maesj_/qdk4z1vm/26/

My solution is an adaptation of a demo that I found on css-tricks adressing a similar kind of situation, but in my case having mutliple buttons has added a bit more complexity to the mix and I'm wondering if anyone has any ideas on how I might improve my code.

Any thoughts are much appreciated!

HTML

<button  class="dyn-gallery-btn" type="button"> one </button>
<button  class="dyn-gallery-btn" type="button"> two </button>

<div class="dyn-slide"> one text
</div>

<div class="dyn-slide"> two text
</div>

<button  class="dyn-gallery-btn" type="button"> three </button>
<button  class="dyn-gallery-btn" type="button"> four </button>

<div class="dyn-slide"> three text
</div>

<div class="dyn-slide"> four text
</div>

CSS

.dyn-slide {
   background: red;
   transition: height .5s ease-in-out;
   height: 0;
   overflow: hidden;
}

.dyn-slide.active {
  height: auto;
  overflow: hidden;
}

.dyn-slide:not(.active) {
  height: 0;
  overflow: hidden;
}

JS

const slides = document.querySelectorAll('.dyn-slide')
const buttons = document.querySelectorAll('.dyn-gallery-btn')

buttons.forEach((button, i) => {
  button.addEventListener('click', () => {
  
    const slide = slides[i]
    const wasActive = slide.classList.contains('active')
    
    slides.forEach(slide => {
      slide.classList.remove('active')
      slide.style.height = 0;
    })
    
    if (!wasActive) {
      slide.classList.add('active')
      
      slide.style.height = 'auto';
  
    var height = slide.clientHeight + 'px';
    slide.style.height = '0px';

    setTimeout(function () {
      slide.style.height = height;
    }, 0);
    
    } else {
    var height = slide.clientHeight + 'px';
     slide.style.height = '0px';
     
     setTimeout(function () {
      slide.style.height = 0;
    }, 0);
      
    }
  })
})

Answer 1

Redundant CSS The rules in .dyn-slide:not(.active) already exist in the .dyn-slide block, so they're not needed - feel free to remove them entirely. (The .dyn-slide.active { height: auto; isn't being used either, since the height is being set directly via JS)

Semicolons or not? To be stylistically consistent and to reduce bugs, either use semicolons everywhere they're appropriate, or (if you consider yourself skilled enough not to run into problems with ASI) don't use semicolons. Choose a style you want, then enforce it.

Use const, avoid var - you're using modern syntax, which is great, but you're using var in a few places. In ES2015+, best to always use const (or, if you need to reassign, let), but var isn't a great idea - it has weird rules like function scope instead of block scope, and automatically assigns its variables to the global object when used on the top level, among other weirdness. (ESLint rule: no-var, I highly recommend using a linter to automatically prompt you to fix code quality smells)

Unused variable You do

} else {
  var height = slide.clientHeight + 'px';

but never use the height variable - remove it. (ESLint rule: no-unused-vars)

Whole else block is redundant Or, remove the whole block there: the element's height has been set to 0 at the beginning of the function, so setting it to 0 again won't change anything.

You can save the last active slide in a variable instead of looping over all slides at the beginning of the click handler.

Trigger reflow instead of using setTimeout Rather than setTimeout, you can trigger a DOM reflow by reading a style property from the DOM, for example, access the offsetHeight of an element. This allows you to code synchronously:

Indentation Some of your indented blocks are not properly aligned. Consider using an IDE to automatically indent correctly - this'll make it easier for you and others to understand the code at a glance.

Refactored:

const slides = document.querySelectorAll('.dyn-slide');
const buttons = document.querySelectorAll('.dyn-gallery-btn');
let lastSlide;

buttons.forEach((button, i) => {
  button.addEventListener('click', () => {
    const slide = slides[i];
    const wasActive = slide.classList.contains('active');
    if (lastSlide) {
      lastSlide.classList.remove('active');
      lastSlide.style.height = 0;
    }
    lastSlide = slide;
    if (wasActive) {
      return;
    }
    slide.classList.add('active');
    slide.style.height = 'auto';
    const newHeight = slide.clientHeight + 'px';
    slide.style.height = '0px';
    slide.offsetHeight; // Force a reflow
    slide.style.height = newHeight;
  })
})

.dyn-slide {
   background: red;
   transition: height .5s ease-in-out;
   height: 0;
   overflow: hidden;
}

.dyn-slide.active {
  /* Height will be set to either auto or 0 by JS */
  overflow: hidden;
}

<button  class="dyn-gallery-btn" type="button"> one </button>
<button  class="dyn-gallery-btn" type="button"> two </button>

<div class="dyn-slide"> one text
</div>

<div class="dyn-slide"> two text
</div>

<button  class="dyn-gallery-btn" type="button"> three </button>
<button  class="dyn-gallery-btn" type="button"> four </button>

<div class="dyn-slide"> three text
</div>

<div class="dyn-slide"> four text
</div>