2
\$\begingroup\$

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');
    }
});

});
\$\endgroup\$
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.