Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

STIJ-196: Added JS namespaces setup. #171

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions components/11-base/base/base.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
var gent_styleguide = (function () { // eslint-disable-line no-unused-vars
var gent_styleguide = gent_styleguide || {};

var base = gent_styleguide.base = (function () { // eslint-disable-line no-unused-vars
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary variable.

Since gent_styleguide is already an object, we can just append properties to it.

gent_styleguide.tabtrap = ...
gent_styleguide.helper = ...

'use strict';

/**
* Generates a tabTrap object
* Generates a tabTrap object.
*
* @param {object} container DOM-element
* @param {object} container DOM-element.
* @constructor
*/
function TabTrap(container) {
Expand Down Expand Up @@ -60,8 +62,44 @@ var gent_styleguide = (function () { // eslint-disable-line no-unused-vars
this.hasFocusables = focusables && focusables.length > 0;
}

/**
* Generates a Helper object.
*
* @constructor
*/
function Helper() {

/**
* Removes a class from a DOM-element.
*
* @param {object} element DOM-element.
* @param [string] className A class name.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My phpstorm doesn't understand this bracket notation.

*/
this.removeClass = function (element, className) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, but are these really helpful?
These functions don't really add much to the existing classList method.

We could potentially use them as a polyfill for IE9, using string.split(' ') on element.className.

if (element.classList) {
element.classList.remove(className);
}
};

/**
* Adds a class from a DOM-element.
*
* @param {object} element DOM-element.
* @param [string] className A class name.
*/
this.addClass = function (element, className) {
if (element.classList) {
element.classList.add(className);
}
else {
element.classList += ' ' + className;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Element.classList is falsy.

}
};
}

return {
TabTrap: TabTrap
TabTrap: TabTrap,
Helper: Helper
};

})();
})(base);
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@
* @file
* Javascript binding of hamburger-menu.functions.js.
*/
(function ($) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place scripts at the bottom of the page

  • faster loading
  • automatically ensures all DOM content is loaded

Also: if you insist on placing them in 'head', use DOMContentLoaded.
No need to wait for the render tree.

window.onload = (function () {
'use strict';

$(window).on('load', function (e) {
var $hamburgerMenu = $('.hamburger-menu');
return function () {
var hamburgerMenu = document.querySelectorAll('.hamburger-menu');

if ($hamburgerMenu.length > 0) {
$hamburgerMenu.loadHamburgerMenu();
for (var i = hamburgerMenu.length; i--;) {
var menu = new hamburger_component.HamburgerMenu(hamburgerMenu[i]); // eslint-disable-line no-undef
menu.loadHamburgerMenu();
Copy link
Contributor

@delrueba delrueba Dec 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of the HamburgerMenu returning functions, but I would load the menu automatically.
We could instead return open() and close() so you can open/close the drawer from code.
I don't know why we'd do this, but it's nice to know we can 😺

}
});

})(jQuery);
};
})();
Original file line number Diff line number Diff line change
@@ -1,36 +1,29 @@
/**
* @file
* Implements a hamburger-menu button combined with
* a slide-in panel for easy mobile navigation.
*
* @authors
* Wim Vantomme
* Bart Delrue
*
*/
(function ($) {
var gent_styleguide = gent_styleguide || {};

var hamburger_component = gent_styleguide.components = (function (context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you can't extend gent_styleguide.components anymore.

I'd use
gent_styleguide.components = gent_styleguide.components || {}
gent_styleguide.components.HamburgerMenu = function(element){} to create a constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or omit 'components' altogether, but that would be a structural decision.

'use strict';

$.fn.extend({

/**
* Creates a jQuery extension function.
*
* @fires event:click
*/
loadHamburgerMenu: function () {
var $drawer = $(this[0]).find('.hamburger-menu__drawer');
var $closeBtn = $drawer.find('.close');
var $overlay = $('.hamburger-menu__overlay');
/**
* Creates a hamburger menu object.
*
* @param {object} element DOM-element.
* @constructor
*/
function HamburgerMenu(element) {
this.loadHamburgerMenu = function () {

var drawer = element.querySelector('.hamburger-menu__drawer');
var closeBtn = drawer.querySelector('.close');
var overlay = element.querySelector('.hamburger-menu__overlay');
var trigger;

if (typeof gent_styleguide === 'undefined') {
console.error('You need to include base.js.'); // eslint-disable-line no-console
return;
}

// TabTrap doens't use jquery opbjects.
var tabTrap = new gent_styleguide.TabTrap($drawer[0]); // eslint-disable-line no-undef
var helper = new base.Helper();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to create a new instance of 'Helper'.
Helper should not be a constructor function.

var tabTrap = new base.TabTrap(drawer); // eslint-disable-line no-undef

/**
* Closes the hamburger menu
Expand All @@ -41,8 +34,9 @@
if (e) {
e.preventDefault();
}
$drawer.removeClass('js-opened');
$overlay.removeClass('js-opened');
helper.removeClass(drawer, 'js-opened');
helper.removeClass(overlay, 'js-opened');

document.removeEventListener('keydown', handleKeyboardInput);
tabTrap.reset();

Expand All @@ -55,7 +49,7 @@
// remove the menu from the tabindex
// jquery .css() doesn't now 'important'
setTimeout(function () {
$drawer.attr('style', 'display: none');
drawer.setAttribute('style', 'display: none');
}, 500);
};

Expand All @@ -71,11 +65,11 @@

// add the menu to the tabindex
// jquery .css() doesn't now 'important'
$drawer.attr('style', 'display: block');
drawer.setAttribute('style', 'display: block');

setTimeout(function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have commented this, took me a moment to figure out why we have to wait for the next tick.

$drawer.addClass('js-opened');
$overlay.addClass('js-opened');
helper.addClass(drawer, 'js-opened');
helper.addClass(overlay, 'js-opened');
});


Expand All @@ -84,7 +78,7 @@
trigger.setAttribute('aria-expanded', true);

// set focus to the menu
$drawer.focus();
drawer.focus();

// handle keyboard input
document.addEventListener('keydown', handleKeyboardInput);
Expand Down Expand Up @@ -144,8 +138,10 @@
*
* @event click
*/
$('.hamburger-menu__toggle').on('click', open);

var toggle = element.querySelector('.hamburger-menu__toggle');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define variables on top

if (toggle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the check.

But

  • the toggle element is not optional, so we shouldn't even reach this far in the code if it's not present.
  • why not check closeBtn and overlay too?

Suggestion:
maybe we should check all compulsory elements at the start of the script.

toggle.addEventListener('click', open);
}

/**
* Indicates that a user has clicked on the closeBtn hamburger menu
Expand All @@ -155,10 +151,17 @@
*
* @event click
*/
$closeBtn.add($overlay).on('click', close);
closeBtn.addEventListener('click', close);
overlay.addEventListener('click', close);

// init the menu as closed on startup
close();
}
});
})(jQuery);

};
}

return {
HamburgerMenu: HamburgerMenu
};

})(hamburger_component);