6
\$\begingroup\$

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);
      
    }
  })
})
\$\endgroup\$
2
\$\begingroup\$

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>

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You write "...and automatically assigns its variables to the global object," What?, The linked article "isn't a great idea" is misinformed and should be removed Some corrections 1 Variables declared with let and const can be in global scope. 2 let and const are NOT function scoped as they are not hoisted and can not be accessed before declaration. 3 var can be declared outside functions and not be in the global scope eg within a module. 4 No mention of strict mode, modules, or hosting and how they affect the behavior of declarations. ...To name a few \$\endgroup\$ – Blindman67 Oct 22 '20 at 19:25
  • \$\begingroup\$ Oops, I missed adding "when declared on the top level". Yeah, the article is pretty sloppy, I just googled and linked to the first result that looked reasonable on the surface. I'll point to AirBnB instead. But the point is still solid, when writing clean, readable code, const should be preferred over var by far. \$\endgroup\$ – CertainPerformance Oct 22 '20 at 19:38
  • \$\begingroup\$ Thank you !! This was super helpful, I was able to clean up my code and get everything working. Really appreciate it! :) \$\endgroup\$ – eliza Nov 11 '20 at 11:30

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.