Issue 5635 - Implement website-default navbar component

static/scss/_navbar.scss

 // This file is part of help.eyeo.com.
 // Copyright (C) 2017 Eyeo GmbH
 //
 // help.eyeo.com is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
 // the Free Software Foundation, either version 3 of the License, or
 // (at your option) any later version.
 //
 // help.eyeo.com is distributed in the hope that it will be useful,
 // but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 10 matching lines @@
 .navbar
 {
   @extend .clearfix;
 }
 
 /* .navbar .navbar-container
  ****************************************************************************/
 
 .navbar-container
 {
   @if ($add-horizontal-navbar-padding)

[juliandoucette, 2017/12/13 16:17:38]

NIT/Suggest: Check if $navbar-padding-x (or similar) is truthy instead?

[ire, 2017/12/14 10:26:36]

I ended up with this solution for a couple of reasons:

1. Both the $navbar-padding-x and $navbar-padding-y should be the same
2. The $add-horizontal-navbar-padding seems like it should be separate because
its true/false value has effects for if there is vertical padding applied.
3. Setting $navbar-padding-x to something falsey doesn't actually mean there
will be no padding applied to the left or right. It's more about *where* that
padding is applied. e.g. see padding on .navbar-branding

Maybe the $add-horizontal-navbar-padding should be renamed? Like I said, it's
more to do with where that padding is applied. 

   {
     @extend .container;
   }
   @else
   {
-    width: $container-width;

[juliandoucette, 2017/12/13 16:17:37]

TOL: It might be cleaner to create a container mixin the accepts width and
padding and use it here/elsewhere

[ire, 2017/12/14 10:26:34]

Great idea! Done.

-    max-width: 100%;
-    margin-right: auto;
-    margin-left: auto;
+    @include container($container-width, 0px);
   }
 }
 
 /* .navbar .navbar-branding
  ****************************************************************************/
 
 .navbar-branding
 {
-  padding-top: $navbar-padding;
-  padding-bottom: $navbar-padding;
+  padding-top: $navbar-padding + ($navbar-padding / 4);
+  padding-bottom: $navbar-padding + ($navbar-padding / 4);
+  line-height: 1;
 
   @if ($add-horizontal-navbar-padding != true)
   {
     padding-right: $navbar-padding;
     padding-left: $navbar-padding;
   }
 }
 
 .navbar-branding img
 {
   height: $navbar-branding-height;
   vertical-align: text-top;
 }
 
 /* .navbar .toggle-navbar-collapse
  ****************************************************************************/
 
 .toggle-navbar-collapse
 {
   display: none;
-  padding-top: $navbar-padding + ($navbar-padding / 2);

[juliandoucette, 2017/12/13 16:17:37]

This doesn't scale with the toggle button height.

-  padding-bottom: $navbar-padding + ($navbar-padding / 2);
+  padding-top: ($navbar-branding-height / 2) + ($navbar-padding / 2);
+  padding-bottom: ($navbar-branding-height / 2) + ($navbar-padding / 2);
 
   @if ($add-horizontal-navbar-padding != true)
   {
     padding-right: $navbar-padding;
     padding-left: $navbar-padding;
   }
 }
 
 .js .toggle-navbar-collapse
 {
   display: block;
 
   @media (min-width: $tablet-breakpoint)
   {
     display: none;
   }
 }
 
 /* .navbar .navbar-collapse
  ****************************************************************************/
 
 .navbar-collapse
 {
   clear: both;
-  padding-top: 0;

[juliandoucette, 2017/12/13 16:17:36]

Why is this necessary?

[ire, 2017/12/14 10:26:36]

Looks like it isn't :/ Must've been from a previous version. Removed.

 
   @media (min-width: $tablet-breakpoint)
   {
     float: right;
     clear: none;
   }
 }
 
 [dir="rtl"] .navbar-collapse
 {
@@ skipping 17 matching lines @@
 
   @media (min-width: $tablet-breakpoint)
   {
     width: auto;
   }
 }
 
 /* .navbar .navbar-nav
  ****************************************************************************/
 
-.navbar .navbar-nav
+@if ($add-horizontal-navbar-padding)
 {
-  @if ($add-horizontal-navbar-padding)

[juliandoucette, 2017/12/13 16:17:36]

A vertical padding dependant on horizontal padding?

[ire, 2017/12/14 10:26:37]

Yes, that vertical padding is only needed if there is also the horizontal
padding

[juliandoucette, 2017/12/14 12:55:41]

Acknowledged.

+  .navbar .navbar-nav
   {
     padding-bottom: $navbar-padding / 2;
-  }
 
-  @media (min-width: $tablet-breakpoint)
-  {
-    padding-bottom: 0;

[juliandoucette, 2017/12/13 16:17:37]

This is only necessary if the previous condition is true?

[ire, 2017/12/14 10:26:36]

Done.

-    
-    @if ($add-horizontal-navbar-padding)
+    @media (min-width: $tablet-breakpoint)
     {
       margin-right: -($navbar-padding / 2);
       margin-left: -($navbar-padding / 2);
+      padding-bottom: 0;
     }
   }
 }
 
 .navbar .navbar-nav li
 {
   @media (min-width: $tablet-breakpoint)
   {
     display: inline-block;
   }
 }
 
 .navbar .navbar-nav a
 {
   display: block;
   padding-top: $navbar-padding / 2;
   padding-bottom: $navbar-padding / 2;
 
   @if ($add-horizontal-navbar-padding != true)
   {
     padding-right: $navbar-padding;
     padding-left: $navbar-padding;
   }
 
   @media (min-width: $tablet-breakpoint)
   {
-    padding-top: $navbar-padding + ($navbar-padding / 2);

[juliandoucette, 2017/12/13 16:17:36]

It seems like both the navbar nav items and the toggle button are taller than
the navbar brand.

It also seems that they will not scale together because they are not based on
the same values.

Here is an example of "based on the same values":

$navbar-brand-height = 2em;
$navbar-item-height = $navbar-brand-height / 2; // or 1em
$navbar-padding = 1em;

$navbar-brand-height + ($navbar-padding * 2) = 4em;
$navbar-item-height + ($navbar-padding * 2) + (($navbar-brand-height -
$navbar-item-height / 2) * 2) = 4em;

Assuming that $navbar-brand-height will be equal to or greater than
$navbar-item-height.

[ire, 2017/12/14 10:26:34]

I've made some changes to address this

[juliandoucette, 2017/12/14 12:55:41]

Would you mind explaining your logic?

[ire, 2017/12/16 09:51:35]

Sure :)

The issue previously was that if the $navbar-branding-height were to change, the
rest of the items in the navbar (the links, the toggle button) would no longer
be vertically aligned to the center. So I changed this by setting the padding
around those items that vertically center it to be based on the
$navbar-barding-height instead of only the $navbar-padding. 

So now, as you scale up/down the height of the navbar-branding, the other items
should maintain their vertical alignment. 

+    padding-top: ($navbar-branding-height / 2) + ($navbar-padding / 2);
     padding-right: $navbar-padding / 2;
-    padding-bottom: $navbar-padding + ($navbar-padding / 2);
+    padding-bottom: ($navbar-branding-height / 2) + ($navbar-padding / 2);
     padding-left: $navbar-padding / 2;
   }
 }