patternjavascriptMinor
jQuery menus, click handlers, and scroll effects for a website
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
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
Instead of this, use viewport units
-
Instead of Core Javascript, use jQuery
can be written as
-
where
-
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.
-
The events bound on
-
As the
should be
-
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 intofunction 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.