0
\$\begingroup\$

I've used multiple tutorials to get the exact look for show/hide using jQuery, but now that I've done it's too repetitive, I've tried to edit it using variables, but it didn't work. Is there a way to avoid repetitiveness here?

$(document).ready(function() {
  $(document).click(function(event) {
    if ($(event.target).is(".info-icon") || $(event.target).is(".info-tab") || ($(".info-tab").has(event.target).length == 1)) {
      if ($(".info-tab").height() != 0 && !$(event.target).is(".info-tab") && !($(".info-tab").has(event.target).length == 1)) {
        $(".info-tab").css("max-height", "0");
      } else {
        if ($(".search-tab").height() != 0) {
          $(".search-tab").css("max-height", "0");
          setTimeout(function() {
            $(".info-tab").css("max-height", "200px");
          }, 300);
        } else {
          $(".info-tab").css("max-height", "200px");
        }
      }
    } else if ($(event.target).is(".search-icon") || $(event.target).is(".search-tab") || ($(".search-tab").has(event.target).length == 1)) {
      if ($(".search-tab").height() != 0 && !$(event.target).is(".search-tab") && !($(".search-tab").has(event.target).length == 1)) {
        $(".search-tab").css("max-height", "0");
      } else {
        if ($(".info-tab").height() != 0) {
          $(".info-tab").css("max-height", "0");
          setTimeout(function() {
            $(".search-tab").css("max-height", "200px");
          }, 300);
        } else {
          $(".search-tab").css("max-height", "200px");
        }
      }
    } else {
      $(".info-tab").css("max-height", "0");
      $(".search-tab").css("max-height", "0");
    }
  });
});

JSFiddle for what it does is here: https://jsfiddle.net/msms92/L9L0tqvp/

\$\endgroup\$
2
\$\begingroup\$

The first thing that comes to my mind is that what you're trying to do is handling all of the click events in just one handler function! Don't!

Use delegate event listeners instead:

$(document)
    .on("click", ".info-icon, .info-tab", function(e) {
       e.stopPropagation();
       // Deal with .info-icon or .info-tab
    })
    .on("click", ".search-icon, .search-tab", function(e) {
       e.stopPropagation();
       // Deal with .search-icon or .search-tab
    })
    .on("click", function(e) {
       // Other clicks
    });

Secondarily, define CSS classes list .info-tab-open and .search-tab-open like these:

.info-tab-open {
    max-height: 200px;
}

(And remember: using magic numbers is bad, especially when styling with JavaScript.)

So you'll end up doing this instead:

var $infoTab = $(".info-tab");
if ($infoTab.hasClass("info-tab-open")) {
    $infoTab.removeClass("info-tab-open");
} else {
    var $searchTab = $(".search-tab");
    if ($searchTab.hasClass("search-tab-open")) {
        $searchTab.removeClass("search-tab-open");
        setTimeout(..., 300);
    } else {
        $infoTab.addClass("info-tab-open");
    }
}

Third: since the handler's code is basically the same and repeating code is a code smell, you'll probably be better off if you use a factory function for the handler:

function toggler(selectedClass, otherClass) {
    return function(e) {
        e.stopPropagation();
        var $selectedTab = $("." + selectedClass);
        if ($selectedTab.hasClass(selectedClass + "-open")) {
            $selectedTab.removeClass(selectedClass + "-open");
        } else {
            var $otherTab = $("." + otherClass);
            if ($otherTab.hasClass(otherClass + "-open")) {
                $otherTab.removeClass(otherClass + "-open");
                setTimeout(..., 300);
            } else {
                $selectedTab.addClass(selectedClass + "-open");
            }
        }
    };
}

$(document)
    .on("click", ".info-icon, .info-tab", toggler("info-tab", "search-tab"))
    .on("click", ".search-icon, .search-tab", toggler("search-tab", "info-tab"))
    .on("click", function() {
        $(".info-tab").removeClass("info-tab-open");
        $(".search-tab").removeClass("search-tab-open");
    });
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

WTF?

Why not just select the block you want with $("CSS _Selector") and use the .show() or .hide() method.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Because I don't want to. It's also not gonna help me avoid repeating the code for each button/tab. \$\endgroup\$ – MSmS Nov 1 '16 at 0:12
  • 1
    \$\begingroup\$ Take at look at my updated fiddle \$\endgroup\$ – Adrian Brand Nov 1 '16 at 0:30
  • 1
    \$\begingroup\$ jsfiddle.net/L9L0tqvp/1 \$\endgroup\$ – Adrian Brand Nov 1 '16 at 0:30
  • 1
    \$\begingroup\$ Okay, it's not exactly what I need, it doesn't hide the other "data-target" when clicked on the second one, but I might get it to work. \$\endgroup\$ – MSmS Nov 1 '16 at 0:35
  • 1
    \$\begingroup\$ Just add a $(".info-wrapper").hide() before the toggle. \$\endgroup\$ – Adrian Brand Nov 1 '16 at 0:36
0
\$\begingroup\$

Don't use the .css method. Create CSS classes and use the class rules to update the display with .addClass and .removeClass, or .toggle

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I tried, .css part isn't the problem. It's the same code for each button/tab, I want to get rid off. \$\endgroup\$ – MSmS Nov 1 '16 at 0:14
  • 2
    \$\begingroup\$ @MSmS It is part of the problem. Your code looks messy to begin with. If you embrace JavaScript's and jQuery's best practices, the solution will come naturally. \$\endgroup\$ – MaxArt Nov 1 '16 at 0:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy