HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

jQuery menus, click handlers, and scroll effects for a website

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
websiteclickmenushandlerseffectsscrolljqueryforand

Problem

So far, I've been using codes based on solutions other people wrote, and I'm trying to learn by modifying those codes. My problem now is that my code ended up too messy (a lot of script tags, jquery mixed with javascript, etc). I would like to know what should I do to clean this mess. Since I'm a beginner, I'm not looking for something radical, but something that would make my code understandable and organized.

This is the page I'm building: website

```

(function() {
"use strict";
var toggles = document.querySelectorAll(".c-hamburger");
for (var i = toggles.length - 1; i >= 0; i--) {
var toggle = toggles[i];
toggleHandler(toggle);
};
function toggleHandler(toggle) {
toggle.addEventListener( "click", function(e) {
e.preventDefault();
(this.classList.contains("is-active") === true) ? this.classList.remove("is-active") : this.classList.add("is-active");
});
}
})();


$(document).ready(function(){
$( "#btnMobileMenu" ).click(function() {
$( "#mbMenu" ).toggle( "slow", function() {

});
});
var heig = $(document).height();
var wid = $(document).width();
$( "#mbMenu" ).css({
height: heig,
width: wid
});
});


//IIF to avoid polluting global namespace
(function() {

$(function() {
//S1 - add click handler to each menu item
$(".anchorLink").each(function(k, v) {
$(v).click(function(e) {
//S2 - build target div id using hash from clicked menu item
var targetId = 'target-' + e.originalEvent.currentTarget.hash.slice(1);
//S3 - scroll document to top offset of target div

Solution

-
Setting the dimenstions - Use CSS

var heig = $(document).height();
var wid = $(document).width();
$("#mbMenu").css({
    height: heig,
    width: wid
});


Instead of this, use viewport units

#mbMenu {
    width: 100vw;
    height: 100vh;
}


-
Instead of Core Javascript, use jQuery

var toggles = document.querySelectorAll(".c-hamburger");
for (var i = toggles.length - 1; i >= 0; i--) {
    var toggle = toggles[i];
    toggleHandler(toggle);
};

function toggleHandler(toggle) {
    toggle.addEventListener("click", function (e) {
        e.preventDefault();
        (this.classList.contains("is-active") === true) ? this.classList.remove("is-active"): this.classList.add("is-active");
    });
}


can be written as

$('.c-hamburger').click(function(e) {
    e.preventDefault();
    $(this).toggleClass('is-active');
});


-
openNav and closeNav can be combined into

function toggleNav(open) {
    $('#questionNav').css('width', open ? '100%' : '0');
}


where open - boolean can be passed to the function to open or close. There is no need to change the opacity as zero-width elements are not visible.

-
There is no need to iterate over multiple elements and bind the event individually. jQuery does that when working on selectors that selects multiple elements.

$('.anchorLink').click(function(e) {
    var targetId = 'target-' + this.hash.slice(1);
    $('body').scrollTop($('#' + targetId).offset().top - 200);
    myEffectsClick(e);
});


-
The events bound on window are not needed to be wrapped inside ready(). Also, multiple statements can be chained and function reference can be used directly as handler.

$(window).scroll(function() {
    $('#navRight .rotate').toggleClass("up", $(window).scrollTop() > 100);
}).on('wheel', myEffectsScroll);


-
As the animate callback is not needed, that can be removed.

$("#mbMenu").toggle("slow", function() {

});


should be

$("#mbMenu").toggle("slow");


-
scrollUp and similarly scrollDown can be written as following. Note that similar code is reused and else if is used.

function scrollUp(windowScrollTop) {
    if (windowScrollTop < 1150) {
        var $el;
        if (windowScrollTop < 350) {
            $el = $('.two');
        } else if (windowScrollTop < 750) {
            $el = $('.three');
        } else if (windowScrollTop < 1150) {
            $el = $('.four');
        }

        $el.css('border-top-color', '#999').animate({
            width: '25px'
        }, 100);
    } else if (windowScrollTop < 1500) {
        $('.one, .two, .three, .four, .five').css('border-top-color', '#fff');
        $('.navbar-nav li a, #navRight a, #footer').css('color', '#fff');

        $('.five').animate({
            width: '25px'
        }, 100);

        $('body').animate({
            background: '#033'
        }, 100);
    }
}

Code Snippets

var heig = $(document).height();
var wid = $(document).width();
$("#mbMenu").css({
    height: heig,
    width: wid
});
#mbMenu {
    width: 100vw;
    height: 100vh;
}
var toggles = document.querySelectorAll(".c-hamburger");
for (var i = toggles.length - 1; i >= 0; i--) {
    var toggle = toggles[i];
    toggleHandler(toggle);
};

function toggleHandler(toggle) {
    toggle.addEventListener("click", function (e) {
        e.preventDefault();
        (this.classList.contains("is-active") === true) ? this.classList.remove("is-active"): this.classList.add("is-active");
    });
}
$('.c-hamburger').click(function(e) {
    e.preventDefault();
    $(this).toggleClass('is-active');
});
function toggleNav(open) {
    $('#questionNav').css('width', open ? '100%' : '0');
}

Context

StackExchange Code Review Q#138898, answer score: 2

Revisions (0)

No revisions yet.