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

Toggling menus based on an ID

Submitted by: @import:stackexchange-codereview··
0
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 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.