patternjavascriptMinor
Toggling menus based on an ID
Viewed 0 times
togglingmenusbased
Problem
I have recently started using browserify and have created a custom module to toggle two menus based on the ID clicked and apply a transparent overlay effect behind the menu. I have got it working but just wanted to get some feedback on weather it can improved performance or coding wise as I am wanting to improve my JS skills.
/**
* Require modules
*/
var dullBackground = require('./dullBackground');
/**
* Export module
*/
module.exports = function() {
'use strict';
/**
* Selectors
*/
var menu = document.getElementById('nav-main');
var panel = document.getElementById('overlay-panel');
var productsPanel = document.getElementById('products-panel');
var systemsPanel = document.getElementById('systems-panel');
var close = document.getElementById('overlay-close');
var activeClass = 'active';
var hideClass = 'hide';
/**
* Methods
*/
function togglePanel(event) {
// Selectors
var elementId = event.target.id;
event.preventDefault();
// Conditions
if(elementId === 'page-products' || elementId === 'page-systems') {
dullBackground();
panel.classList.add(activeClass);
}
if(elementId === 'page-products') {
productsPanel.classList.remove(hideClass);
}
if(elementId === 'page-systems') {
systemsPanel.classList.remove(hideClass);
}
}
function closePanel(event) {
// Actions
panel.classList.remove(activeClass);
event.preventDefault();
// Conditions
if(!productsPanel.classList.contains(hideClass)) {
productsPanel.classList.add(hideClass);
}
if(!systemsPanel.classList.contains(hideClass)) {
systemsPanel.classList.add(hideClass);
}
}
/**
* Events/APIs/init
*/
menu.addEventListener('click', togglePanel, false);
close.addEventListener('click', closePanel, false);
};Solution
First time doing a review, so bear with me.
I've been thinking about how to make any changes to your code, but had a difficult time finding things that could be optimized. Overall, I think you did a great job.
Selectors - this may be a style preference, but I tend to lump everything under one var statement (delimited by commas), rather than having to write var each time.
Methods - I chose to pass the
Methods -
Again, this is my first stab at a code review, so I'm happy to hear your feedback as well as any other feedback from the community.
I've been thinking about how to make any changes to your code, but had a difficult time finding things that could be optimized. Overall, I think you did a great job.
Selectors - this may be a style preference, but I tend to lump everything under one var statement (delimited by commas), rather than having to write var each time.
Methods - I chose to pass the
elementId into document.getElementById rather than having to write additional if statements to check specifically for either id. You're already checking to see what event.target.id is returning, might as well use it again.Methods -
closePanel, I created a function called checkClassListAndAddClass very long, but at least you know what it's doing. This function takes the selector as an argument, checks to see if the selector contains a class, and then returns the add class. I added a variable to that function to help remove some repetitive code.Again, this is my first stab at a code review, so I'm happy to hear your feedback as well as any other feedback from the community.
/**
* Require modules
*/
var dullBackground = require('./dullBackground');
module.exports = function () {
'use strict';
/**
* Selectors
*/
var menu = document.getElementById('nav-main'),
overlay = document.getElementById('overlay-panel'),
close = document.getElementById('overlay-close'),
productsPanel = document.getElementById('page-products'),
systemsPanel = document.getElementById('page-systems'),
activeClass = 'active',
hideClass = 'hide';
/**
* Methods
*/
function togglePanel(event) {
var elementId = event.target.id;
event.preventDefault();
if (elementId === ('page-products' || elementId === 'page-systems')) {
dullBackground();
overlay.classList.add(activeClass);
document.getElementById(elementId).classList.remove(hideClass);
}
}
function closePanel(event) {
event.preventDefault();
overlay.classList.remove(activeClass);
checkClassListAndAddClass(productsPanel);
checkClassListAndAddClass(systemsPanel);
}
function checkClassListAndAddClass(selector) {
var selClassList = selector.classList;
if (!selClassList.contains(hideClass)) {
return selClassList.add(hideClass);
}
}
/**
* Events/APIs/init
*/
menu.addEventListener('click', togglePanel, false);
close.addEventListener('click', closePanel, false);
};Code Snippets
/**
* Require modules
*/
var dullBackground = require('./dullBackground');
module.exports = function () {
'use strict';
/**
* Selectors
*/
var menu = document.getElementById('nav-main'),
overlay = document.getElementById('overlay-panel'),
close = document.getElementById('overlay-close'),
productsPanel = document.getElementById('page-products'),
systemsPanel = document.getElementById('page-systems'),
activeClass = 'active',
hideClass = 'hide';
/**
* Methods
*/
function togglePanel(event) {
var elementId = event.target.id;
event.preventDefault();
if (elementId === ('page-products' || elementId === 'page-systems')) {
dullBackground();
overlay.classList.add(activeClass);
document.getElementById(elementId).classList.remove(hideClass);
}
}
function closePanel(event) {
event.preventDefault();
overlay.classList.remove(activeClass);
checkClassListAndAddClass(productsPanel);
checkClassListAndAddClass(systemsPanel);
}
function checkClassListAndAddClass(selector) {
var selClassList = selector.classList;
if (!selClassList.contains(hideClass)) {
return selClassList.add(hideClass);
}
}
/**
* Events/APIs/init
*/
menu.addEventListener('click', togglePanel, false);
close.addEventListener('click', closePanel, false);
};Context
StackExchange Code Review Q#81745, answer score: 2
Revisions (0)
No revisions yet.