Skip to main content
6 of 6
simplify CSS

##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 -->