0

so i'm a bit confused as why the code im working off isn't working as expected, so any help would be good.

so let me explain im working on an expanding/ collapsing menu, when the a tag is clicked a class is added and css is added to make this happen, and also the class is removed from the other links.

CSS code:

a {
max-height: 0;
overflow: hidden;
transition: 0.5s ease-in-out
}
a.clicked {
max-height: 600px;
}

html code:

<ul>

<li><a onclick='mobileMenu(event)'>link 1</a><li>

<li><a onclick='mobileMenu(event)'>link 2</a><li>

<li><a onclick='mobileMenu(event)'>link 3</a><li>

<li><a onclick='mobileMenu(event)'>link 4</a><li>

</ul>

for this post i kept the html pretty simple but if it needs to be more detailed let me know.

Each link has a onclick attached.

Javascript code:

function mobileMenu(event) {

        event.currentTarget.classList.toggle("clicked");

        [].forEach.call(document.querySelectorAll('ul li a'), function(a){

            a.classList.remove('clicked');

        });

}

so this code mostly works where you click a link is adds 'clicked' class and also removes the 'clicked' class from any other links except the current link, which is good because im using this as a collapse-able menu so you click another link it opens it while also closing any other link currently open.

My problem is is the js code above to add the clicked class i'm using a '.toggle' but this only adds the 'clicked' class but does not toggle it. I want to be able to toggle the 'clicked' class and also remove it from others links. so im not sure if its a simple oversight on my part but im not sure where im going wrong and why the '.toggle' isn't actually toggling the class.

Thanks.

2
  • <a onclick='mobileMenu'> will not do anything, you're just referencing a function and doing nothing with it. You probably wanted <a onclick='mobileMenu(event)'> but it would be even better to avoid inline handlers entirely Commented Aug 31, 2020 at 21:01
  • oh right ye sorry ahah i actually have it like that in my code ... my mistake for not adding it correctly
    – ddd
    Commented Sep 1, 2020 at 11:18

2 Answers 2

2

You can try a if statement. It will always remove the checked from all links, and if the event.target is not checked, it will add the class checked.

function mobileMenu(event) {
    
  const checked = event.currentTarget.classList.contains('clicked')
  
  [].forEach.call(document.querySelectorAll('ul li a'), function(a){
    
    a.classList.remove('clicked');
    
  });

  if (checked) return;
  
  event.currentTarget.classList.toggle("clicked");
}
0
2

As per comment the right way to add event listeners is using .addEventListener().

In the event listener you can search for an anchor with a class clicked and if any remove the class. After you can add the class to the current element:

document.querySelectorAll('ul li a').forEach(function(ele, idx) {
    ele.addEventListener('click', function(e) {
        var clickedEle = document.querySelector('ul li a.clicked');
        if (clickedEle != null)
            clickedEle.classList.remove('clicked');
        this.classList.add('clicked');
    })
});
a {
    max-height: 0;
    overflow: hidden;
    transition: 0.5s ease-in-out
}
a.clicked {
    max-height: 600px;
    color: blue;
}
<ul>
    <li><a>link 1</a><li>
    <li><a>link 2</a><li>
    <li><a>link 3</a><li>
    <li><a>link 4</a><li>
</ul>

1
  • Also, I'll add that to keep your list dynamic, you might not want to set a hard value for max-height in the 'clicked' class. Any part of the list added that is longer than 600px will be cut off. That is why it's better to set the maxHeight's value to the event's scroll height using Javascript. Commented Sep 1, 2020 at 12:51

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