Overriding scrolling behavior for navigation elements

Question

I am new to JavaScript, and trying to ensure the i write the best possible code instead of "copy & paste".

I hope i am asking correctly, want to know if my code I have written is done correctly or could it be done better, the code works, so nothing wrong with the end result, just not sure if I'm on the right track.

$(document).ready(function() {
"use strict";
var menu_btn = $('a.menu-trigger');
var menu = $('#main_nav > ul');
var w = $(window).width();
// Adding the ScrollTo Function on the menu items
$('#main_nav > ul a[href^="#"],.logo a[href^="#"]').on('click', function(e) {
 var target = $( $(this).attr('href') );
 if(target.length ) {
 e.preventDefault();
 $('html, body').animate({
 scrollTop: target.offset().top
 }, 1000);
 }
 if (w < 768) {
 $('#main_nav > ul').css('display', 'none');
 }
});
// Create the Sticky Header
$(window).scroll(function() {
 if ($(this).scrollTop() > 1){ 
 $('.header-inner').addClass("sticky");
 }else{
 $('.header-inner').removeClass("sticky");
 }
});
// Mobile Menu Functionality
menu_btn.on('click', function(e) {
 e.preventDefault();
 menu.slideToggle ();
});
$(window).resize(function () {
 if (w > 768 && menu.is(':hidden')) {
 menu.removeAttr('style');
 }
});
});

Answer 1

One potential problem I see is that you initialize the w variable once at $(document).ready() time and then you use that value in several event handlers. But the window width is a dynamic value that can be changed over time so your event handlers can be using a stale value for w. You should probably be fetching $(window).width() anytime you need it so it is always correct.

---

You can also replace:

$('#main_nav > ul').css('display', 'none');

with this:

menu.hide();

since you've already cached the value of $('#main_nav > ul') and .hide() is a shortcut for .css('display', 'none');.

---

You could also probably replace this entire code section:

if (w < 768) {
 $('#main_nav > ul').css('display', 'none');
}

with a CSS rule using media queries.

---

menu.removeAttr('style'); is pretty harsh (removing all possible styles). If you're just trying to show the menu again when the width has exceeded a desired value, then you can just do this:

$(window).resize(function () {
 if ($(window).width() > 768) {
 menu.show();
 }
});

Note, there is no need to check .is(':hidden') because calling .show() on something that is already visible just does nothing and .is(':hidden') is a slightly expensive operation because it has to check the visibility of every single parent object.