5
\$\begingroup\$

I've written some jQuery code that works, but I'm a novice in jQuery and I want to know how to improve my code if possible.

I know I have 3 near identical blocks of code. If I create a function, the code will be the same. I'm looking for more accurate code and am trying to reduce my code into fewer lines.

JSFiddle

    $('#click-me-idiomas').click(function () {
        var idiomasVisible = $("#menu-idiomas").is(":visible");
        var coleccionesVisible = $("#menu-colecciones").is(":visible");
        var nosotrosVisible = $("#menu-nosotros").is(":visible");
        if (coleccionesVisible || nosotrosVisible) {
            if (coleccionesVisible) {
                $('#click-me-colecciones').removeClass('active');
                $('#click-me-idiomas').addClass('active');
                $("#menu-colecciones").slideUp('slow', function () {
                    $('#menu-idiomas').slideToggle('slow');
                });
            }
            if (nosotrosVisible) {
                $('#click-me-nosotros').removeClass('active');
                $('#click-me-idiomas').addClass('active');
                $("#menu-nosotros").slideUp('slow', function () {
                    $('#menu-idiomas').slideToggle('slow');
                });
            }
        } else {
            if (idiomasVisible) {
                $("#menu-idiomas").slideToggle('slow', function () {
                    $('#click-me-idiomas').removeClass('active');
                });
            } else {
                $('#click-me-idiomas').addClass('active');
                $("#menu-idiomas").slideToggle('slow');
            }
        }

    });

    $('#click-me-colecciones').click(function () {
        var idiomasVisible = $("#menu-idiomas").is(":visible");
        var coleccionesVisible = $("#menu-colecciones").is(":visible");
        var nosotrosVisible = $("#menu-nosotros").is(":visible");
        if (idiomasVisible || nosotrosVisible) {
            if (idiomasVisible) {
                $('#click-me-idiomas').removeClass('active');
                $('#click-me-colecciones').addClass('active');
                $("#menu-idiomas").slideUp('slow', function () {
                    $('#menu-colecciones').slideToggle('slow');
                });
            }
            if (nosotrosVisible) {
                $('#click-me-nosotros').removeClass('active');
                $('#click-me-colecciones').addClass('active');
                $("#menu-nosotros").slideUp('slow', function () {
                    $('#menu-colecciones').slideToggle('slow');
                });
            }
        } else {
            if (coleccionesVisible) {
                $("#menu-colecciones").slideToggle('slow', function () {
                    $('#click-me-colecciones').removeClass('active');
                });
            } else {
                $('#click-me-colecciones').addClass('active');
                $("#menu-colecciones").slideToggle('slow');
            }
        }

    });

    $('#click-me-nosotros').click(function () {
        var idiomasVisible = $("#menu-idiomas").is(":visible");
        var coleccionesVisible = $("#menu-colecciones").is(":visible");
        var nosotrosVisible = $("#menu-nosotros").is(":visible");
        if (idiomasVisible || coleccionesVisible) {
            if (idiomasVisible) {
                $('#click-me-idiomas').removeClass('active');
                $('#click-me-nosotros').addClass('active');
                $("#menu-idiomas").slideUp('slow', function () {
                    $('#menu-nosotros').slideToggle('slow');
                });
            }
            if (coleccionesVisible) {
                $('#click-me-colecciones').removeClass('active');
                $('#click-me-nosotros').addClass('active');
                $("#menu-colecciones").slideUp('slow', function () {
                    $('#menu-nosotros').slideToggle('slow');
                });
            }
        } else {
            if (nosotrosVisible) {
                $("#menu-nosotros").slideToggle('slow', function () {
                    $('#click-me-nosotros').removeClass('active');
                });
            } else {
                $('#click-me-nosotros').addClass('active');
                $("#menu-nosotros").slideToggle('slow');
            }
        }

    });
\$\endgroup\$
1
\$\begingroup\$

I've written in my way. You can see it with this link http://jsfiddle.net/968pA/8/ .

HTML :

<header id="masthead" class="container site-header" role="banner">
  <nav role="navigation">
    <ul class="menu">
      <li><a href="#" id="click-me-languages" data-toggle="menu-languages">LANGUAGES</a></li>
      <li><a href="#" id="click-me-books" data-toggle="menu-books">BOOKS</a></li>
      <li><a href="#" id="click-me-about" data-toggle="menu-about">ABOUT US</a></li>
    </ul>
  </nav>
</header>
<div class="container">
  <div class="menu-list" id="menu-languages">
    <div class="row clearfix">
      <div class="col-3">
        <ul>
          <li><a href="">English</a></li>
          <li><a href="">Spanish</a></li>
          <li><a href="">Français</a></li>
          <li><a href="">Deutsch</a></li>
        </ul>
      </div>
    </div>
  </div>
  <div class="menu-list" id="menu-books">
    <div class="row clearfix">
      <div class="col-3">
        <ul>
          <li><a href="">Book #1</a></li>
          <li><a href="">Book #2</a></li>
          <li><a href="">Book #3</a></li>
          <li><a href="">Book #4</a></li>
          <li><a href="">Book #5</a></li>
        </ul>
      </div>
    </div>
  </div>
  <div class="menu-list" id="menu-about">
    <div class="row clearfix">
      <div class="col-3">
        <ul>
          <li><a href="">About us</a></li>
          <li><a href="">Contact</a></li>
          <li><a href="">Blog</a></li>
        </ul>
      </div>
    </div>
  </div>
</div>

CSS (same as original) :

* { 
    -webkit-box-sizing: border-box; /* Safari/Chrome, other WebKit */
    -moz-box-sizing: border-box;    /* Firefox, other Gecko */
    box-sizing: border-box;         /* Opera/IE 8+ */
}
body {margin:0; padding:0;}
a {text-decoration:none;}
.container { padding-left:15px; padding-right:15px; margin:0 auto; max-width:1170px; }
.col-3 { width:285px; }

.col-3 {
    float:left;
    position: relative;
    min-height: 1px;
    padding-right: 15px;
    padding-left: 15px;
}

.site-header { background-color:#00AF85; }
.site-header li { display:inline; font-family:'Open Sans', sans-serif; font-weight:700; line-height:70px; padding-right:18px; }
.site-header a { color:#fff; }
.site-header a:hover,
.site-header a.active { color:#403F41; }

#menu-languages,
#menu-books,
#menu-about { background-color:#fff; display:none; font-family:'Open Sans', sans-serif; font-size:16px; font-weight:700; text-transform:uppercase; }

#menu-languages .col-12, #menu-books .col-12, #menu-about .col-12 { height:50px; }

#menu-languages li, #menu-books li, #menu-about li { padding-bottom:5px; padding-top:5px; }
#menu-languages a, #menu-books a, #menu-about a { line-height:20px; }

.clearfix:after {
  content: "";
  display: table;
  clear: both;
}

JS :

function getActiveLink() {
    var $activeLink = $('#masthead').find('a.active:first');
    return ($activeLink.length > 0)? $activeLink: null;
}

function getActiveMenu() {
    var $activeMenu = $('.menu-list:visible:first');
    return ($activeMenu.length > 0)? $activeMenu: null;
}

$('#click-me-languages, #click-me-books, #click-me-about').click(function (event) {
    var $thisLink = $(event.currentTarget);
    var menu = $thisLink.data('toggle');
    var $thisMenu = $('#' + menu);
    var $activeLink = getActiveLink();
    var $activeMenu = getActiveMenu();
    if ($thisMenu.is($activeMenu)) {
        $activeLink.removeClass('active');
        $activeMenu.slideUp('slow');
    }else if ($activeMenu === null) {
        $thisLink.addClass('active');
        $thisMenu.slideDown('slow');        
    }else {
        $activeMenu.slideUp('slow', function() {
            $activeLink.removeClass('active');
            $thisLink.addClass('active');
            $thisMenu.slideDown('slow');
        });
    }
});

I tried to remove duplicated code and merge the events to one, So I need to get menu and link that being active, then I can do whatever to the menu and the link. My code isn't the best but I think you can learn from it. I added class to div menus and added data-toggle to links so that I can get active link and active menu easier.

\$\endgroup\$
  • \$\begingroup\$ Links can go away or temporarily fail. Please add the code to your answer. \$\endgroup\$ – RubberDuck Jul 10 '14 at 11:49
  • \$\begingroup\$ I like it, but: there doesn't seem to be much point in adding the data-toggle. It could be renamed to something more meaningful (data-menu-id?), or it could be removed altogether. In the latter case, replace with var menu = $thisLink.attr("id").substr("click-me-".length);. In fact, the menu variable is used only once, and it isn't a complicated calculation, nor does it really add clarity to the code. The expression could be pasted directly into the selector: var $thisMenu = $("#menu-" + $thisLink.attr("id").substr("click-me-".length));. \$\endgroup\$ – Schism Jul 10 '14 at 15:33
  • \$\begingroup\$ Kamonwat Sangudsub thanks a lot!!!! And Schism, thanks to you too. I will take your tip and I will put it on the code :) \$\endgroup\$ – hormigaz Jul 11 '14 at 9:15

Your Answer

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

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