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.