CODEWITHSUNDEEP
javascriptcssdomecmascript-6accordion
ImranR
ImranR

Reputation: 516

Toggle issue with Accordions/Tabs using JS (ES6)

I am having a slight issue with the following accordion/tab layout:

https://jsfiddle.net/drj3m5tg/

As you will see, on mobile the "+" icon on the right stays as a "-" icon when opening and closing other tabs. And on desktop view, the tabs sometimes need to be clicked twice to reveal the content below. I checked in chrome inspector, and can see the active class is not being removed until clicking again. Is there any way to fix this behaviour using JS?

const accordion = document.getElementsByClassName("accordion");
const panel = document.getElementsByClassName("panel");

for(let i = 0; i < accordion.length; i++){
  accordion[i].addEventListener('click', function(){
    removeOpen();
    // add active class to clicked element and open class to next slibling
    const toggleResult = this.classList.toggle('active');
    this.nextElementSibling.classList.toggle('panel-open', toggleResult);


  });
};

 // remove open class for all elements
function removeOpen (){
    for(let i = 0; i < panel.length; i++) {
       panel[i].classList.remove('panel-open');
    }
};

Upvotes: 2

Views: 1572

Answers (1)

ibrahim mahrir
ibrahim mahrir

Reputation: 31692

It's because you have to remove the active class from the other accordion buttons. When you do that, you'll fall into another problem which is that toggling is no longer working. So I suggest you approach it like this (refactoring the whole thing):

(function() {                                                           // not really necessary (just to hide our variables from the outside scope)
    const accordion = document.getElementsByClassName("accordion");     // the .accordion buttons (no need for panels, we can get them using nextElementSibling)
    let current = -1;                                                   // the index of the current active accordion element (-1 indicate that currently no element is active)

    for (let i = 0; i < accordion.length; i++) {
        accordion[i].addEventListener('click', function() {             // when clicking a .accordion element
            if (i !== current && current !== -1) {                      // if this is not the currently active element (i !== current), and if there is a currently active element (current !== -1)
                accordion[current].classList.remove('active');          // then desactivate the currently active element
                accordion[current].nextElementSibling.classList.remove('panel-open'); // ...
            }
            this.nextElementSibling.classList.toggle('panel-open');     // now toggle the current element
            current = this.classList.toggle('active') ? i : -1;         // ... (if this element is toggled on, then set current to be this element, if it is toggled off then set current to -1 as there will be no active elements)
        });
    };
})();

JSFiddle

EDIT:

current holds a value that is the index of the current accordion element that has the class active. So 0 <= current < accordion.length. There could be no active element (all accordion elements are closed), so we need a value to indicate that. The value must not be in the aforementioned range. It could be anything: null, false, "oompa loompa", ... Since it is universal to use -1 (as indexOf does to indicate inexistance), I chose it too.

As for why are we using it?. Well, instead of removing the active classes from all the elements everytime an element is clicked, we just keep track of the active element, and each time another element get clicked we only remove them from one element (whose index is stored in current). Since current also indicates that there is no element, we have to test first (current !== -1).

When an element is clicked, we also wants to check if it is not the active element (i !== -1). Because if we don't do that, then we'll remove the active classes inside if and the toggles will add them back. So without this test, when clicking the active element, it will stay active.

Upvotes: 1

Related Questions