STIJ-196: Added JS namespaces setup. #171

meirege · 2017-12-01T15:16:37Z

Description

Proposed solution for JS namespaces. What do you think @Jeroen005 @delrueba @wim-vantomme ?

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have updated the CHANGELOG accordingly.
I have read the CONTRIBUTING document.

delrueba

I've refactored the whole thing in a new commit.
I haven't incorporated all my comments into it, so check it out and let's combine the best of both ;)

delrueba · 2017-12-03T09:48:23Z

components/11-base/base/base.js

-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


Unnecessary variable.

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

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

delrueba · 2017-12-03T09:58:39Z

components/31-molecules/building-blocks/hamburger-menu/hamburger-menu.binding.js

@@ -2,15 +2,16 @@
 * @file
 * Javascript binding of hamburger-menu.functions.js.
 */
-(function ($) {
+


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.

delrueba · 2017-12-03T10:08:43Z

components/11-base/base/base.js

+     * @param {object} element DOM-element.
+     * @param [string] className A class name.
+     */
+    this.removeClass = function (element, className) {


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.

delrueba · 2017-12-03T10:09:59Z

components/11-base/base/base.js

+        element.classList.add(className);
+      }
+      else {
+        element.classList += ' ' + className;


Element.classList is falsy.

delrueba · 2017-12-03T10:21:35Z

components/31-molecules/building-blocks/hamburger-menu/hamburger-menu.functions.js

      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();


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

delrueba · 2017-12-03T11:35:15Z

components/11-base/base/base.js

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


My phpstorm doesn't understand this bracket notation.

delrueba · 2017-12-03T11:38:36Z

components/31-molecules/building-blocks/hamburger-menu/hamburger-menu.binding.js

-      $hamburgerMenu.loadHamburgerMenu();
+    for (var i = hamburgerMenu.length; i--;) {
+      var menu = new hamburger_component.HamburgerMenu(hamburgerMenu[i]); // eslint-disable-line no-undef
+      menu.loadHamburgerMenu();


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 😺

delrueba · 2017-12-03T11:42:43Z

components/31-molecules/building-blocks/hamburger-menu/hamburger-menu.functions.js

@@ -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 () {


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

delrueba · 2017-12-03T11:45:29Z

components/31-molecules/building-blocks/hamburger-menu/hamburger-menu.functions.js

@@ -144,8 +138,10 @@
       *
       * @event click
       */
-      $('.hamburger-menu__toggle').on('click', open);
-
+      var toggle = element.querySelector('.hamburger-menu__toggle');


define variables on top

delrueba · 2017-12-03T11:47:55Z

components/31-molecules/building-blocks/hamburger-menu/hamburger-menu.functions.js

-      $('.hamburger-menu__toggle').on('click', open);
-
+      var toggle = element.querySelector('.hamburger-menu__toggle');
+      if (toggle) {


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.

…lpers.

Jeroen005 · 2017-12-08T15:20:49Z

components/main_cli.scss

@@ -0,0 +1,17 @@
+// inject:settings


remove this file ;-)

STIJ-196: Added JS namespaces setup.

e9e5ba1

meirege self-assigned this Dec 1, 2017

meirege requested review from Jeroen005, wim-vantomme and delrueba December 1, 2017 15:16

delrueba suggested changes Dec 3, 2017

View reviewed changes

delrueba added 2 commits December 3, 2017 12:56

STIJ-196: Updated teaser__overlay-link to match WCAG 2.4.9.

ddfa152

STIJ-196: Refactored global JS namespace setup.

ac6f860

meirege added this to the 2.8.0 milestone Dec 5, 2017

STIJ-196: Added prefixedAnimationEvent function to gent_styleguide.he…

4db71d6

…lpers.

meirege added the enhancement label Dec 8, 2017

STIJ-196: Adding babel, browserify.

ceecec3

meirege removed this from the 2.8.0 milestone Dec 8, 2017

Jeroen005 suggested changes Dec 8, 2017

View reviewed changes

STIJ-196: Removed main_cli.scss from git because it is a generated file.

5063fac

meirege added this to the 2.8.0 milestone Dec 12, 2017

meirege modified the milestones: 2.8.0, 2.9.0 Jan 18, 2018

meirege modified the milestones: 2.9.0, 3.0.0 Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STIJ-196: Added JS namespaces setup. #171

STIJ-196: Added JS namespaces setup. #171

meirege commented Dec 1, 2017 •

edited

Loading

delrueba left a comment

delrueba Dec 3, 2017

delrueba Dec 3, 2017

delrueba Dec 3, 2017

delrueba Dec 3, 2017

delrueba Dec 3, 2017

delrueba Dec 3, 2017

delrueba Dec 3, 2017 •

edited

Loading

delrueba Dec 3, 2017

delrueba Dec 3, 2017

delrueba Dec 3, 2017

Jeroen005 Dec 8, 2017

meirege Dec 12, 2017

STIJ-196: Added JS namespaces setup. #171

Are you sure you want to change the base?

STIJ-196: Added JS namespaces setup. #171

Conversation

meirege commented Dec 1, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

delrueba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delrueba Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meirege commented Dec 1, 2017 •

edited

Loading

delrueba Dec 3, 2017 •

edited

Loading