Issue 2675 - Implemented responsive header for web.eyeo.com

static/js/scripts.js

-jQuery(function()

[saroyanm, 2016/04/08 16:56:10]

Note: I think if we could use Javascript instead of Jquery we will be able to
get rid of Jquery from some point, because it's only website we are using Jquery
and lots of Jquery based suspicious libraries, but I'll let that up to you,
while that can be done in separate review as well.
Also I'm not sure if the animations provide that much value.
Maybe make sense also discuss the scroll animation removal with Designers before
doing that, but I assume that will save a lot of headache in future if we can
get rid of Jquery.

[juliandoucette, 2016/04/25 19:08:56]

- I agree that we should get rid of jquery
- But I think that is outside of the scope of this review
- I also think that we should remove/change the animation
- But I think that is outside of the scope of this review too (I intentionally
kept it for that reason)

Since you are the module owner, please agree and make new issues for these, or
disagree in response to this comment.

[saroyanm, 2016/05/06 14:49:28]

I think if we will decide to get rid of animation function we can also implement
Javascript solution in this review while most of the page is affected and I
think it should be easy to use Javascript while we only have few jquery script
and the only tricky is animation also if we will consider to remote the to-top
element as you suggested I will really be in favor to have Javascript in current
review, otherwise we will implement something and reimpolement that in
Javascript in separate review I think having it in this review is justified in
this case.

[juliandoucette, 2016/05/11 18:11:06]

Ok, I will implement this without jQuery.

-{
-  var $toTop = jQuery("#to-top");

[saroyanm, 2016/04/08 16:56:09]

Why to use dollar sign in front of the variable names ?

[saroyanm, 2016/04/08 16:56:10]

The element is missing, it was removed.

[juliandoucette, 2016/04/25 19:08:56]

This is a standard practice used to identify a value that is an instance of
jQuery.

($() returns an instance of jQuery)

[juliandoucette, 2016/04/25 19:08:56]

Good point :D . I didn't notice that.

I think we should leave it out. (I know, that is inconsistent with _staying in
scope_ above.) What do you think?

[saroyanm, 2016/05/06 14:49:27]

Acknowledged.

[saroyanm, 2016/05/06 14:49:28]

I agree I'm not sure if it provides much value, but please remove all related
code as well.

[juliandoucette, 2016/05/11 18:11:06]

Will do.

+(function(){
+  function addListener(target, event, callback)
+  {
+    if (target.addEventListener)
+      return target.addEventListener(event, callback, false);
+    else
+      return target.attachEvent("on" + event, callback);
+  }
 
-  $toTop.click(function ()
+  function onLoad(callback)
   {
-    jQuery("body,html").animate({
-        scrollTop: 0
-    }, 800);
-    return false;
+    if (document.addEventListener)
+      return addListener(document, "DOMContentLoaded", callback);
+    else
+      return addListener(window, "load", callback);
+  }
+
+  onLoad(function()
+  {
+    // expand & contract fixed header
+    var header = document.getElementById("header");
+    var headerPadding = 13;
+    addListener(window, "scroll", function()
+    {
+      var scrollY = window.scrollY || document.documentElement.scrollTop;
+      if (scrollY < headerPadding)
+        header.className = "top";
+      else
+        header.className = "";
+    });
+    // open & close header menu (on small screens)
+    var menu = document.getElementById("menu");
+    var menuButton = document.getElementById("header-hamburger");
+    addListener(menuButton, "click", function()
+    {
+      if (menu.className === "open")
+      {
+        menu.className = "";
+        menu.setAttribute("aria-expanded", false);
+      }
+      else
+      {
+        menu.className = "open";
+        menu.setAttribute("aria-expanded", true);
+      }
+    });
   });
-
-  var $window = jQuery(window);
-  var $header = jQuery("#header");
-  var headerPadding = 13;
-
-  $window.scroll(function()
-  {
-    if ($window.scrollTop() > headerPadding)
-    {
-      if ($header.hasClass("top"))
-      {
-        $header.removeClass("top");
-        $toTop.show();
-      }
-    } 
-    else
-    {
-      if (!$header.hasClass("top"))
-      {
-        $header.addClass("top");
-        $toTop.hide();
-      }
-    }
-  });
-
-  var $menu = jQuery("#menu");
-
-  jQuery('#header-hamberger').click(function()

[saroyanm, 2016/05/06 14:49:28]

Detail: it's "hamburger" also please use double quote '"' sign for consistency.

[juliandoucette, 2016/05/11 18:11:06]

Nice catch :)

-  {
-    if ($menu.hasClass("open")) {

[saroyanm, 2016/04/08 16:56:09]

Detail: please move opening curly brace on it's own line.

[juliandoucette, 2016/04/25 19:08:56]

Nice catch.

-      $menu.removeClass("open");
-      $menu.attr("aria-expanded", false);
-    }
-    else
-    {
-      $menu.addClass("open");
-      $menu.attr("aria-expanded", true);
-    }
-  })
-
-});
+}());