Click handler running just once

When I click on a hamburger menu, I want it to pull out the aside element, which it does only the first time the event is fired. Subsequent clicks are monitored in devtools and show that

  1. the event is still attached
  2. the class open is struggling to be added to the class list but something seems to be preventing it.

I know I’m missing something slight so I need an extra pair of eyes

$(document).ready(function() {
		$('#menu-container').on('click', function(e) {
			$('#menu-container').toggleClass('open');

			$('aside').css('width', '18%');
			
			$('.open').on('click', function() {
				$('.open').removeClass('open');

				$('aside').css('width', 0);
			});
		});
	})

Full demo fiddle can be found here

I’m kind of tired, but I have made a jsfiddle of my website.

https://jsfiddle.net/Strider64/o6s8d8aL/5/

Here’s the jQuery:

$(function() {
  $('.icon').on('click', function(e) {
    e.preventDefault();
    if ($('ul#myTopnav').hasClass('responsive')) {
      $('ul#myTopnav').removeClass('responsive');
    } else {
      $('ul#myTopnav').addClass('responsive');
    }
  });
});

here’s the HTML

<nav class="container nav-bar">
  <ul class="topnav" id="myTopnav">
    <li><a class="top-link" href="#">&nbsp;</a></li>
    <li><a href="index.php">Home</a></li>
    <li><a href="about.php">About</a></li>
    <li><a href="https://www.triviaintoxication.com/">Trivia</a></li>
    <li><a href="contact.php">Contact</a></li>
    <li><a href="countdown.php">Baseball Begins</a></li>
    <li class="icon">
      <a href='#'>&#9776;</a>
    </li>
  </ul>
</nav>

and the rest is on the link.

Looking at it quicky you might have to use e.preventDefault(); to prevent the anchor tag from firing.
HTH John

1 Like

After each click on the #menu-container, you’re attaching a new click handler to the same element (which is now .open as well) to remove that open class again. This 2nd click handler will remain attached even if the open class got removed again, piling up more and more click handlers to remove the class. So subsequent clicks on the element will trigger all of these handlers, toggling the class and immediately removing it again. Try something like this instead:

$(document).ready(function() {
  $menuContainer = $('#menu-container')
  $aside = $('aside')
  
  $menuContainer.on('click', function(e) {
    $menuContainer.toggleClass('open')
		
    if ($menuContainer.hasClass('open')) {
      $aside.css('width', '18%')
    } else {
      $aside.css('width', 0)
    }
  })
})
1 Like

This is quite some brain fuck.

I reasoned that since the class open doesn’t exist always, the handler gets deleted as it is not attached to anybody hence the necessity to attach it each time it’s created.

I’ve just discovered that this too works

$('#menu-container').click(() => $('#menu-container,aside').toggleClass('open'));

As long as you add this to the stylesheet

aside.open {
	width: 18%;
}

I was looking at something like $(\${this},aside`).toggleClass(‘open’)` but I guess object and string combination selectors are not allowed by jQuery.

1 Like

event handlers are attached to elements, not classes. if you remove a class, its element–and its handler–are still present.

Yeah, using CSS for styling would certainly be the cleanest solution. :-)

As @Dormilich said, the element is still the same whether it has that class or not. What you could do though is using event delegation, like

$(document).on('click', '#menu-container.open', function () {
  $('aside').css('width', 0)
})

This will only trigger the event on the menu container when it also has the open class. Still, using CSS where possible is always better.

I confess to having some level of difficulty with assimilating this. In theory, classes are selectors and elements are DOM objects but technically, classes are elements, or in my understanding at least. Because there are no classes without elements, if you were to say[quote=“m3g4p0p, post:7, topic:260765”]

$(document).on('click', '#menu-container.open', function () {
  $('aside').css('width', 0)
})

[/quote]

This handler is attached to the class open (even though theoretically, this isn’t correct) but if the handler doesn’t fire if that element doesn’t have class open, doesn’t it mean the handler was attached to the class? At that point, the handler will be garbage collected assuming we were in classic OOP languages. So I think classes should also be called pseudo elements even though it’s tautology in theory.

Well, which is why this doesn’t work. The syntax looks beautiful though. Shame it’s vain beauty. Remember the aside has width 0 on page load, so there’s no way we need to attach a handler to #menu-container. Would probably work in reverse i.e if we needed to drag aside in on click. And even then the toggle effect won’t work. By the way, what’s the difference between this

and this $('#menu-container.open').click(function(){}); ? I think event delegation allows future elements long after DOM is ready to get the handler. Event delegation is sort of an escrow for intending or aspiring elements. Am I right?

EDIT: Just seen this so I guess, never mind. @m3g4p0p why was it important in the current scenario though?

Fast and effective:

$(document).ready(function() {
		$('#menu-container').on('click', function(e) {
			$('#menu-container').toggleClass('open');
      if($('#menu-container').hasClass('open')){
      $('aside').css('width', '18%');
      }else{
      $('aside').css('width', '0%');
      }			
		});
	})

Well no, the handler is actually attached to the document, but gets triggered only for (children) elements that match the given selector. A basic VanillaJS implementation would look like

function delegate (context, type, selector, callback) {
  context.addEventListener(type, function (event) {
    var target = event.target

    if (target.matches(selector)) {
      callback.call(target, event)
    }
  })
}

delegate(document, 'click', '.open', function () {
  this.classList.remove('open')
})

I don’t know why it doesn’t work for you, probably an implementation detail… but you get the idea.

I just wanted to point out that it’s possible to attach event listeners that are sensitive to DOM changes, in quite a similar way you originally intended. Your additional class + CSS route is certainly preferable though in this case.

Ah! Every time I think I’ve spent enough time writing JS to be called a paragon in it, then I come here, you always have a way of making me feel so small :weary: :weary:. So what you’re saying in essence is that all handlers are implicitly attached to the document itself. I’ve tried viewing the event listeners tab plus the methods from this thread just to be double sure. No handler object were returned on the document object. Tried it on elements I attached handlers to and it worked. I was even skeptical from the first time I read your comment because I think it’ll be too much overhead if ALL handlers were attached to the document. I mean, it sort of makes sense with the nested condition but it still sounds like an overkill to me. Especially when you could attach the handler directly.

One last thing, the context switch here is off the charts.

You must be some really cheeky guy.

No, just in that particular case we are explicitly attaching a listener to the document – but we might choose another, closer parent element as well. This is a common pattern if you have e.g. a list, and instead of iterating over all li elements and binding separate listeners, you’d just bind one single listener to the containing ul element. This is especially useful if the list can change dynamically.

You are right that you should not start binding all listeners to the document – this is just the last resort if you can’t find any other way, e.g. to if you want to add a specific listener to all links on a page, or if the element in question is itself at the root level.

This was just to demonstrate how jQuery handles event delegation; the important part is that you can access the element that triggered the event with event.target, from where the event bubbles up to the element to which the listener was actually attached. So in the actual addEventListener() callback, this would refer to context in the above example.

I understand. I’m just not used to that type of maneuver. So according to this

in a situation like this, if I had attached the handler to the parent container and ensured the handler only fired (or became potent) when the target matched class nobody, I would have saved myself the stress of porting the function all over the place? One of the major problems in that situation was the thisvehemently refused to switch contexts. I repeatedly tried calling the handler like in this implementation but it refused to work. I’m still too traumatised by that episode to reattempt reviving the dilemma using this current solution.

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.