5
\$\begingroup\$

I have a working restaurant menu system - as the user selects a menu it slides into view. The build is responsive, and I've used the 'Debounced Resize() jQuery Plugin'. However, in implementing the responsive code, I feel my code has become clunky, and it needs refactoring.

The hope here is that I'll receive some constructive criticism and some pointers on how to improve my code, which will in turn grow my knowledge base.

HTML

<section id="menu">
 <div class="menu-container">
 <ul class="menu__nav">
 <li class="menu__nav-item"><a href="#menu-name-1">Menu Title 1</a></li>
 <li class="menu__nav-item"><a href="#menu-name-2">Menu Title 2</a></li>
 <li class="menu__nav-item"><a href="#menu-name-2">Menu Title 2</a></li>
 </ul>
 <!-- Menu 1 -->
 <div id="#menu-name-1" class="menu__main cf">
 <h2 class="menu__course-title">Menu Title 1</h2>
 <div class="menu__col-wrap">
 <div class="menu__col">
 <div class="menu__col">
 <div class="menu__subcol">
 <!-- The Menu -->
 </div>
 <div class="menu__subcol">
 <!-- The Menu -->
 </div>
 <div class="menu__subcol">
 <!-- The Menu -->
 </div>
 </div>
 </div>
 </div>
 <div class="menu__image"></div>
 </div>
 <!-- Menu 2 etc... -->
 </div>
</section>

jQuery

$(window).smartresize(function () {
 var $mmenu = $('.menu__main');
 if ($mmenu.length > 0) {
 var $mnav = $('.menu__nav'),
 $mnav_a = $mnav.find('a'),
 m = parseInt($mnav.outerHeight(true), 10),
 $contain = $mmenu.closest('.menu-container'),
 h = 0,
 l = 0;
 // check and store if .active element exists
 var active = $mmenu.not(':first').is('.active');
 // if it doesn't exists
 if (!active) {
 $mmenu.hide() // hide all
 .removeClass('active') // remove the active class from any that have it
 .first() // select the first only
 .addClass('active') // give it the active class
 .show(); // and show it 
 $mnav_a.eq(0).addClass('active');
 $mmenu.each(function(z) {
 var $this = $(this);
 $this.css('height','auto');
 $this.css({
 zIndex: z+1,
 position: 'absolute',
 top: m + "px",
 left: l,
 height: (parseInt($this.outerHeight(false), 10) + m) + "px"
 });
 l = l - $this.outerWidth();
 });
 $contain.height($mmenu.eq(0).height());
 } else {
 $mmenu.each(function(z) {
 var $this = $(this);
 $this.css('height','auto');
 $this.css({
 zIndex: z+1,
 position: 'absolute',
 top: m + "px",
 height: (parseInt($this.outerHeight(false), 10) + m) + "px"
 });
 });
 var $new_contain = $('.menu__main.active').height();
 $contain.height($new_contain);
 }
 $mnav_a.on('click', function(e) {
 e.preventDefault();
 var $this = $(this);
 if (!$this.hasClass('active')) {
 $mmenu.stop(true, true);
 $mnav_a.removeClass('active');
 $this.addClass('active');
 $mmenu.filter($('.active'))
 .removeClass('active')
 .fadeOut(250)
 .animate({left:l+"px"}, 250);
 var $target = $mmenu.filter($this.attr('href'));
 $contain.animate({height:$target.height()}, 250);
 $target.addClass('active')
 .fadeIn(250)
 .animate({left:0}, 250);
 }
 $this.blur();
 });
 }
});

Working Example

CodePen

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Apr 15, 2014 at 11:01
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Some of your code is redundant. These two lines are mutually exclusive. Especially since you add "active" to the first menu item, which is the only one which would satisfy !active.

 if (!active) {
 .removeClass('active') // remove the active class from any that have it

Makes me wonder if the "if (!active)" branch can be removed entirely.

Revised CodePen: http://codepen.io/anon/pen/FHfAi

answered Apr 15, 2014 at 12:14
\$\endgroup\$
3
  • \$\begingroup\$ Nice cleanup, +1 \$\endgroup\$ Commented Apr 15, 2014 at 12:54
  • \$\begingroup\$ Thank you for the help, mutually exclusive code is something I haven't considered before. In terms of the if (!active) {} block, this is in place to fix an issue I had when a menu other than the first was selected and the screen was resized \$\endgroup\$ Commented Apr 22, 2014 at 8:28
  • 1
    \$\begingroup\$ @SamHolguin I noticed that when I was working on the revised CodePen. By the way, I love the way your menu works. Nice job. \$\endgroup\$ Commented Apr 22, 2014 at 11:12

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.