6
\$\begingroup\$

I've created a simple toggle menu in jQuery, which is capable of specifying a breakpoint, but I'm afraid my code is too complicated. How can I make it simpler?

I also have a question about performance of code on window resize. I want to invoke a function only once if the window's width is smaller than a given breakpoint. Should I use some boolean variable, or is there a better solution?

Here's a pen.

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu'),
      $window = $(window),
      breakpoint = 800;
  
  function changeMenuStyle() {
    if ($window.width() < breakpoint) {
      $menuBtn.show();
      $nav.addClass('toggled').hide();
    } else {
      $menuBtn.hide();
      $nav.removeClass('toggled').show();
    }
  }
  
  changeMenuStyle();
  
  $window.on('resize',function() {
    changeMenuStyle();
  });
  
  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});
* {
  box-sizing: border-box;
}

body {
  background: #485373;
  font-family: "Lato", sans-serif;
  font-weight: 300;
}

.container {
  max-width: 940px;
  margin: 0 auto;
}

.menu-link {
  display: block;
  color: #fff;
  background: #202332;
  width: 100%;
  padding: 20px;
  text-decoration: none;
}
.menu-link i {
  display: inline-block;
  margin: 0 10px;
}

.menu {
  overflow: hidden;
}
.menu i {
  display: block;
  font-size: 25px;
  padding: 10px 0;
}
.menu ul {
  width: 100%;
}
.menu li {
  width: 20%;
  float: left;
  text-align: center;
}
.menu a {
  display: block;
  padding: 20px;
  width: 100%;
  background: #323A51;
  color: #fff;
  text-decoration: none;
  border-bottom: 2px transparent solid;
}
.menu a:hover {
  background: #202332;
  border-bottom: 2px #fff solid;
}
.menu.toggled i {
  display: inline-block;
  margin-right: 10px;
  padding: 0;
  font-size: 15px;
}
.menu.toggled li {
  width: 100%;
}
.menu.toggled a {
  padding: 10px;
}

main {
  padding: 50px;
  color: #fff;
}
main h1 {
  font-size: 30px;
  font-weight: 400;
  line-height: 40px;
  margin: 10px 0;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="container">
  <header>
    <a href="#menu" class="menu-link"><i class="fa fa-bars"></i>Menu</a>
    <nav id="menu" class="menu" role="navigation">
      <ul>
        <li><a href="#"><i class="fa fa-home"></i>Home</a></li>
        <li><a href="#"><i class="fa fa-info"></i>About</a></li>
        <li><a href="#"><i class="fa fa-book"></i>Services</a></li>
        <li><a href="#"><i class="fa fa-users"></i>Team</a></li>
        <li><a href="#"><i class="fa fa-envelope"></i>Contact</a></li>
      </ul>
    </nav>
    <!-- /#menu -->
  </header>
  <main>
    <h1>Toggle menu</h1>
    <p>Vestibulum tempus at ipsum a finibus. Mauris pharetra eleifend odio quis lacinia. Pellentesque ullamcorper, dolor vel congue tincidunt, mi nisl suscipit metus, nec bibendum elit ligula vitae leo. Integer volutpat semper iaculis. Mauris fermentum, enim ac finibus scelerisque, magna nibh volutpat dolor, vitae sodales mi augue molestie augue. Aliquam ultricies eget mi vel viverra. Morbi id tortor purus. Vivamus sed pretium justo. Fusce porttitor eu neque quis volutpat. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur ultrices metus a aliquet tincidunt. Ut pellentesque dui nec ipsum fringilla ultricies. </p>
  </main>
</div>
<!-- /.container -->

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Feedback

The code looks pretty good. The Javascript code waits until the DOM is ready, then caches references to DOM elements in variables before using those variables in the functions later on.

I initially thought some of the CSS could be consolidated (e.g. many common styles under .menu-link and .menu a but then I realized that might be tricky with the SCSS...

I want to invoke a function only once if the window's width is smaller than a given breakpoint. Should I use some boolean variable, or is there a better solution?

One technique for this is to use a debounced function callback. There are jQuery plugins for that, or that article mentions ways to achieve it on your own (like I suggested in this SO answer).

Suggestions

Toggle the display using .toggle()

The .show() and .hide() calls can be replaced with a single call to .toggle(). Also, using the second form of .toggleClass(), the class name toggled can be added/removed depending on whether the menu button is being displayed or not (i.e. if the width is less than the break point). Thus the function changeMenuStyle() can be reduced to two lines (or one if you move the conditional out of a variable):

function changeMenuStyle() {
    var LTBreakpoint = $window.width() < breakpoint;
    $menuBtn.toggle(LTBreakpoint).toggleClass('toggled', LTBreakpoint);
}

Remove closure just to call function

The resize callback can simply be the name of the function - i.e.

$window.on('resize',function() {
    changeMenuStyle();
});

Can be simplified to:

$window.on('resize', changeMenuStyle);

Though be aware that the event parameter would get passed to the function, which could have adverse affects if the function is expecting other parameters.

CSS media query

I should have mentioned this, as Rotora did- CSS media queries can be used to eliminate much of the Javascript code (like you did in the CSS for this post).

Adding this CSS:

/* default settings ( width >= 800px) */  
#menu{
    display: block;
}
[href="#menu"] {
    display: none;
}
/* only when screen width is less than 800px */
@media (max-width: 799px) { 
    #menu{
        display: none;
    }
    [href="#menu"] {
        display: block;
    }
}

Can allow the JS to be simplified to:

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu');

  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});

$(document).ready(function() {
  var $menuBtn = $('[href="#menu"]'),
      $nav = $('#menu');
 
  $menuBtn.on('click',function(e) {
    e.preventDefault();
    $nav.slideToggle();
  });
});
* {
  box-sizing: border-box;
}

body {
  background: #485373;
  font-family: "Lato", sans-serif;
  font-weight: 300;
}

.container {
  max-width: 940px;
  margin: 0 auto;
}

.menu-link {
  display: block;
  color: #fff;
  background: #202332;
  width: 100%;
  padding: 20px;
  text-decoration: none;
}
.menu-link i {
  display: inline-block;
  margin: 0 10px;
}

.menu {
  overflow: hidden;
}
.menu i {
  display: block;
  font-size: 25px;
  padding: 10px 0;
}
.menu ul {
  width: 100%;
}
.menu li {
  width: 20%;
  float: left;
  text-align: center;
}
.menu a {
  display: block;
  padding: 20px;
  width: 100%;
  background: #323A51;
  color: #fff;
  text-decoration: none;
  border-bottom: 2px transparent solid;
}
.menu a:hover {
  background: #202332;
  border-bottom: 2px #fff solid;
}
.menu.toggled i {
  display: inline-block;
  margin-right: 10px;
  padding: 0;
  font-size: 15px;
}
.menu.toggled li {
  width: 100%;
}
.menu.toggled a {
  padding: 10px;
}

main {
  padding: 50px;
  color: #fff;
}
main h1 {
  font-size: 30px;
  font-weight: 400;
  line-height: 40px;
  margin: 10px 0;
}
/* default settings ( width >= 800px) */  
#menu{
    display: block;
}
[href="#menu"] {
    display: none;
}
/* only when screen width is less than 800px */
@media (max-width: 799px) { 
    #menu{
        display: none;
    }
    [href="#menu"] {
        display: block;
    }
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="container">
  <header>
    <a href="#menu" class="menu-link"><i class="fa fa-bars"></i>Menu</a>
    <nav id="menu" class="menu" role="navigation">
      <ul>
        <li><a href="#"><i class="fa fa-home"></i>Home</a></li>
        <li><a href="#"><i class="fa fa-info"></i>About</a></li>
        <li><a href="#"><i class="fa fa-book"></i>Services</a></li>
        <li><a href="#"><i class="fa fa-users"></i>Team</a></li>
        <li><a href="#"><i class="fa fa-envelope"></i>Contact</a></li>
      </ul>
    </nav>
    <!-- /#menu -->
  </header>
  <main>
    <h1>Toggle menu</h1>
    <p>Vestibulum tempus at ipsum a finibus. Mauris pharetra eleifend odio quis lacinia. Pellentesque ullamcorper, dolor vel congue tincidunt, mi nisl suscipit metus, nec bibendum elit ligula vitae leo. Integer volutpat semper iaculis. Mauris fermentum, enim ac finibus scelerisque, magna nibh volutpat dolor, vitae sodales mi augue molestie augue. Aliquam ultricies eget mi vel viverra. Morbi id tortor purus. Vivamus sed pretium justo. Fusce porttitor eu neque quis volutpat. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur ultrices metus a aliquet tincidunt. Ut pellentesque dui nec ipsum fringilla ultricies. </p>
  </main>
</div>
<!-- /.container -->

\$\endgroup\$
3
\$\begingroup\$

There is no real reason to use JavaScript here. Using a CSS media query you can do this in a few lines:

#menu {
   display: block;
}
[href="#menu"] {
   display: none;
}

@media (max-width: 800px) {  // rules only apply if screen width is up to 800px wide
   #menu {
      display: none;
   }
   [href="#menu"] {
      display: block;
   }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Good idea, though only one of those elements should have display: none \$\endgroup\$ Commented Dec 21, 2017 at 20:46
  • \$\begingroup\$ @SamOnela Oops, yes, I misread the original code. I've corrected it. \$\endgroup\$
    – RoToRa
    Commented Dec 22, 2017 at 7:42
-1
\$\begingroup\$

Just a little simple than yours, you can change your function for the same action.

  function changeMenuStyle() {
  $menuBtn.hide();    
  $nav.removeClass('toggled');
     if ($window.width() < breakpoint) {
        $menuBtn.show();
        $nav.addClass('toggled');
    } 
  }
\$\endgroup\$
1
  • 5
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ Commented Nov 23, 2016 at 14:37

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.