5
\$\begingroup\$

This code is used to toggle some dropdowns. I've added the click event on the document to close the dropdown on click outside, but I'm not sure if I'm doing it the right way. I want it to be optimized for better performance.

Let me know if you have a better way to bind/unbind the events.

$('.header-top-bar .toggle').each(function (index) {
  var $this = $(this),
      $parent = $this.parent(),
      $dropdown = $this.siblings('.toggle-content');

  $this.on('click.toggle' + index, function (event) {
    event.preventDefault();

    $dropdown.toggleClass('isOpen');

    // Outside
    $document.on('click.toggle' + index, function (event) {
      if (!$(event.target).closest($parent).length) {
        $dropdown.removeClass('isOpen');
        $document.off('click.toggle' + index);
      }
    });
  });
});

jsFiddle

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

First, regarding readability, I suggest changing the following names:

  • $dropdown -> $content
  • .toggle (class) -> .togglable
  • .toggle (namespace) -> .toggling
  • .isOpen -> isToggleOpen

On the other hand this can be achieved with reduced code (and less internal working impact), with only one event binded:

var $togglables = $('.togglable-content');
$(document).on('click.toggling', function (event) {
  var $target = $(event.target),
  $content = $target.siblings('.togglable-content');
  if (!$target.hasClass('isTogglableOpen')) {
    $togglables.not($content).removeClass('isTogglableOpen');
    $content.toggleClass('isTogglableOpen');
    return false;
  }
});

Here is the modified fiddle.

\$\endgroup\$
11
  • \$\begingroup\$ Very nice answer... \$\endgroup\$ Commented Jan 20, 2016 at 23:49
  • \$\begingroup\$ Or, if you like one-liners ... \$\endgroup\$ Commented Jan 21, 2016 at 1:11
  • \$\begingroup\$ @Roamer-1888 Oh yes, I retain good ideas: 1) reducing internal work by using next() rather than siblings(); 2) the trick of toggling involved element first inside of the not() definition. On the other hand, I prefer keeping $toggables definition outside, to evaluate it only once. \$\endgroup\$
    – cFreed
    Commented Jan 21, 2016 at 1:22
  • \$\begingroup\$ @cFreed, yes for sure, I'm not sure I would go quite that far in deployed code. \$\endgroup\$ Commented Jan 21, 2016 at 1:42
  • \$\begingroup\$ @cFreed Thanks for your input. I am not sure what has more impact on the dom, but from your answer I see that every time a click happens you run the code to check if the click is on the togglable button. Isn't that worst in terms of performance? On my original code I only bind the click on dom once the togglable is open and unbind it once is closed. I would prefer something more similar to that. Also there's an issue with your code, if you click inside the togglable content it closes. Thanks again. \$\endgroup\$ Commented Jan 22, 2016 at 0:39
0
\$\begingroup\$

I ended using this code, once the dropdown is closed no more events run.

$('.header-top-bar .toggle').on('click', function (event) {
  event.preventDefault();

  var $this = $(this),
      $content = $this.next('.toggle-content');

  if (!$content.hasClass('isOpen')) {
    $content.addClass('isOpen');

    setTimeout(function () {
      $document.on('click.headerToggle', function (event) {
        if (!$(event.target).closest($content).length) {
          $content.removeClass('isOpen');
          $document.off('click.headerToggle');
        }
      });
    }, 10);
  }
});

Thank you all for your help.

Fiddle

\$\endgroup\$
2

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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