refactor58727-collapsed-sidebar-flyout-menu-items-don-t-appear-in-1200px-screen-size

15 files changed, 131 insertions, 118 deletions

diff --git a/app/assets/javascripts/breakpoints.js b/app/assets/javascripts/breakpoints.js
index 7951348d8b2..7feb0293ec2 100644
--- a/app/assets/javascripts/breakpoints.js
+++ b/app/assets/javascripts/breakpoints.js
@@ -1,4 +1,13 @@
+/*
+ * Breakpoint values should be updated to Bootstrap 4 values
+ *
+ * The BS4 `xl` property has been added ahead of this refactor
+ * and temporarily duplicates the existing BS3 `lg` property.
+ *
+ * https://gitlab.com/gitlab-org/gitlab-ce/issues/5674
+ */
 export const breakpoints = {
+  xl: 1200,
   lg: 1200,
   md: 992,
   sm: 768,
@@ -6,13 +15,42 @@ export const breakpoints = {
 };
 
 const BreakpointInstance = {
-  windowWidth: () => window.innerWidth,
+  memoWindowWidth: undefined,
+  windowWidth() {
+    return (this.memoWindowWidth = this.memoWindowWidth || window.innerWidth);
+  },
+  memoBreakpointSize: undefined,
   getBreakpointSize() {
-    const windowWidth = this.windowWidth();
+    if (this.memoBreakpointSize) return this.memoBreakpointSize;
 
+    const windowWidth = this.windowWidth();
     const breakpoint = Object.keys(breakpoints).find(key => windowWidth > breakpoints[key]);
 
-    return breakpoint;
+    return (this.memoBreakpointSize = breakpoint);
+  },
+
+  reset() {
+    this.memoWindowWidth = undefined;
+    this.memoBreakpointSize = undefined;
+
+    return this;
+  },
+
+  is(...testBreakpoints) {
+    const actual = this.getBreakpointSize();
+    const testLength = testBreakpoints.length - 1;
+
+    const regexpString = testBreakpoints.reduce((accum, bp, index) => {
+      accum += bp;
+      if (index !== testLength) accum += '|';
+      return accum;
+    }, '');
+
+    return new RegExp(`^(${regexpString})$`).test(actual);
+  },
+
+  isNot(...testBreakpoints) {
+    return !this.is(...testBreakpoints);
   },
 };
 
diff --git a/app/assets/javascripts/contextual_sidebar.js b/app/assets/javascripts/contextual_sidebar.js
index 67fcdd082a2..805288ba584 100644
--- a/app/assets/javascripts/contextual_sidebar.js
+++ b/app/assets/javascripts/contextual_sidebar.js
@@ -2,107 +2,77 @@ import $ from 'jquery';
 import Cookies from 'js-cookie';
 import _ from 'underscore';
 import bp from './breakpoints';
-import { parseBoolean } from '~/lib/utils/common_utils';
-
-// NOTE: at 1200px nav sidebar should not overlap the content
-// https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24555#note_134136110
-const NAV_SIDEBAR_BREAKPOINT = 1200;
 
 export default class ContextualSidebar {
+  #cookieKey = 'sidebar_collapsed';
+  #cookieDuration = 365 * 10;
+  #styleClass = 'sidebar-collapsed';
+
   constructor() {
-    this.initDomElements();
-    this.render();
+    this.$sidebar = $('.nav-sidebar');
+    this.behaviourClass = 'js-sidebar-collapsed';
+    this.classes = `${this.#styleClass} ${this.behaviourClass}`;
   }
 
-  initDomElements() {
-    this.$page = $('.layout-page');
-    this.$sidebar = $('.nav-sidebar');
+  toggle(shouldCollapse = !this.$sidebar.hasClass(this.behaviourClass), saveCookie) {
+    const isMobile = bp.is('xs', 'sm');
 
-    if (!this.$sidebar.length) return;
+    this.$sidebar.toggleClass(this.classes, shouldCollapse);
+    this.$page.toggleClass('page-with-icon-sidebar', isMobile ? true : shouldCollapse);
+    this.$overlay.toggleClass('mobile-nav-open', isMobile ? !shouldCollapse : false);
 
-    this.$innerScroll = $('.nav-sidebar-inner-scroll', this.$sidebar);
-    this.$overlay = $('.mobile-overlay');
-    this.$openSidebar = $('.toggle-mobile-nav');
-    this.$closeSidebar = $('.close-nav-button');
-    this.$sidebarToggle = $('.js-toggle-sidebar');
+    if (saveCookie) this.#setCookie(shouldCollapse);
   }
 
-  bindEvents() {
-    if (!this.$sidebar.length) return;
+  retoggle() {
+    this.toggle(this.$sidebar.hasClass(this.behaviourClass));
+  }
 
-    this.$openSidebar.on('click', () => this.toggleSidebarNav(true));
-    this.$closeSidebar.on('click', () => this.toggleSidebarNav(false));
-    this.$overlay.on('click', () => this.toggleSidebarNav(false));
-    this.$sidebarToggle.on('click', () => {
-      if (!ContextualSidebar.isDesktopBreakpoint()) {
-        this.toggleSidebarNav(!this.$sidebar.hasClass('sidebar-expanded-mobile'));
-      } else {
-        const value = !this.$sidebar.hasClass('sidebar-collapsed-desktop');
-        this.toggleCollapsedSidebar(value, true);
-      }
-    });
-
-    $(window).on('resize', () => _.debounce(this.render(), 100));
+  open() {
+    this.toggle(false);
   }
 
-  // TODO: use the breakpoints from breakpoints.js once they have been updated for bootstrap 4
-  // See documentation: https://design.gitlab.com/regions/navigation#contextual-navigation
-  static isDesktopBreakpoint = () => bp.windowWidth() >= NAV_SIDEBAR_BREAKPOINT;
-  static setCollapsedCookie(value) {
-    if (!ContextualSidebar.isDesktopBreakpoint()) {
-      return;
-    }
-    Cookies.set('sidebar_collapsed', value, { expires: 365 * 10 });
+  close() {
+    this.toggle(true);
   }
 
-  toggleSidebarNav(show) {
-    const breakpoint = bp.getBreakpointSize();
-    const dbp = ContextualSidebar.isDesktopBreakpoint();
+  render() {
+    if (!this.$sidebar.length) return;
+
+    this.#initDomElements();
+    this.#bindEvents();
 
-    this.$sidebar.toggleClass('sidebar-expanded-mobile', !dbp ? show : false);
-    this.$overlay.toggleClass(
-      'mobile-nav-open',
-      breakpoint === 'xs' || breakpoint === 'sm' ? show : false,
-    );
-    this.$sidebar.removeClass('sidebar-collapsed-desktop');
+    this.retoggle();
   }
 
-  toggleCollapsedSidebar(collapsed, saveCookie) {
-    const breakpoint = bp.getBreakpointSize();
-    const dbp = ContextualSidebar.isDesktopBreakpoint();
+  #initDomElements() {
+    this.$page = $('.layout-page');
 
-    if (this.$sidebar.length) {
-      this.$sidebar.toggleClass('sidebar-collapsed-desktop', collapsed);
-      this.$sidebar.toggleClass('sidebar-expanded-mobile', !dbp ? !collapsed : false);
-      this.$page.toggleClass(
-        'page-with-icon-sidebar',
-        breakpoint === 'xs' || breakpoint === 'sm' ? true : collapsed,
-      );
-    }
+    this.$overlay = $('.mobile-overlay');
+    this.$openSidebar = $('.toggle-mobile-nav');
+    this.$closeSidebar = $('.close-nav-button');
+    this.$sidebarToggle = $('.js-toggle-sidebar');
+  }
 
-    if (saveCookie) {
-      ContextualSidebar.setCollapsedCookie(collapsed);
-    }
+  #bindEvents() {
+    this.$openSidebar.on('click', () => this.open());
+    this.$closeSidebar.on('click', () => this.close());
+    this.$overlay.on('click', () => this.close());
+    this.$sidebarToggle.on('click', () => this.toggle(undefined, true));
 
-    requestIdleCallback(() => this.toggleSidebarOverflow());
+    $(window).on('resize', _.debounce(() => this.#reset(), 300));
   }
 
-  toggleSidebarOverflow() {
-    if (this.$innerScroll.prop('scrollHeight') > this.$innerScroll.prop('offsetHeight')) {
-      this.$innerScroll.css('overflow-y', 'scroll');
-    } else {
-      this.$innerScroll.css('overflow-y', '');
-    }
+  #reset() {
+    const before = bp.getBreakpointSize();
+    const after = bp.reset().getBreakpointSize();
+
+    if (before !== after) this.retoggle();
   }
 
-  render() {
-    if (!this.$sidebar.length) return;
+  #setCookie(value) {
+    if (bp.isNot('xl')) return;
 
-    if (!ContextualSidebar.isDesktopBreakpoint()) {
-      this.toggleSidebarNav(false);
-    } else {
-      const collapse = parseBoolean(Cookies.get('sidebar_collapsed'));
-      this.toggleCollapsedSidebar(collapse, true);
-    }
+    Cookies.set(this.#cookieKey, value, { expires: this.#cookieDuration });
   }
 }
diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js
index 2b6af9060d1..0f9c1409177 100644
--- a/app/assets/javascripts/fly_out_nav.js
+++ b/app/assets/javascripts/fly_out_nav.js
@@ -8,6 +8,7 @@ let currentOpenMenu = null;
 let menuCornerLocs;
 let timeoutId;
 let sidebar;
+let sidebarCollpasedClass;
 
 export const mousePos = [];
 
@@ -29,7 +30,7 @@ const setHeaderHeight = () => {
 };
 
 export const isSidebarCollapsed = () =>
-  sidebar && sidebar.classList.contains('sidebar-collapsed-desktop');
+  sidebar && sidebar.classList.contains(sidebarCollpasedClass);
 
 export const canShowActiveSubItems = el => {
   if (el.classList.contains('active') && !isSidebarCollapsed()) {
@@ -171,8 +172,9 @@ export const subItemsMouseLeave = relatedTarget => {
   }
 };
 
-export default () => {
+export default (collapsedClass) => {
   sidebar = document.querySelector('.nav-sidebar');
+  sidebarCollpasedClass = collapsedClass;
 
   if (!sidebar) return;
 
diff --git a/app/assets/javascripts/layout_nav.js b/app/assets/javascripts/layout_nav.js
index 4314e5e1afb..6d174f3511f 100644
--- a/app/assets/javascripts/layout_nav.js
+++ b/app/assets/javascripts/layout_nav.js
@@ -17,9 +17,9 @@ function initDeferred() {
 
 export default function initLayoutNav() {
   const contextualSidebar = new ContextualSidebar();
-  contextualSidebar.bindEvents();
+  contextualSidebar.render();
 
-  initFlyOutNav();
+  initFlyOutNav(contextualSidebar.behaviourClass);
 
   // We need to init it on DomContentLoaded as others could also call it
   $(document).on('init.scrolling-tabs', () => {
diff --git a/app/assets/stylesheets/framework/contextual_sidebar.scss b/app/assets/stylesheets/framework/contextual_sidebar.scss
index 3238b01c6c0..e67e0d2a701 100644
--- a/app/assets/stylesheets/framework/contextual_sidebar.scss
+++ b/app/assets/stylesheets/framework/contextual_sidebar.scss
@@ -121,8 +121,9 @@
   box-shadow: inset -1px 0 0 $border-color;
   transform: translate3d(0, 0, 0);
 
-  &:not(.sidebar-collapsed-desktop) {
-    @media (min-width: map-get($grid-breakpoints, sm)) and (max-width: map-get($grid-breakpoints, sm)) {
+
+  &:not(.sidebar-collapsed) {
+    @media (max-width: map-get($grid-breakpoints, sm)) {
       box-shadow: inset -1px 0 0 $border-color, 2px 1px 3px $dropdown-shadow-color;
     }
   }
@@ -130,10 +131,6 @@
   @mixin collapse-contextual-sidebar {
     width: $contextual-sidebar-collapsed-width;
 
-    .nav-sidebar-inner-scroll {
-      overflow-x: hidden;
-    }
-
     .badge.badge-pill:not(.fly-out-badge),
     .sidebar-context-title,
     .nav-item-name {
@@ -153,14 +150,10 @@
     }
   }
 
-  &.sidebar-collapsed-desktop {
+  &.sidebar-collapsed {
     @include collapse-contextual-sidebar;
   }
 
-  &.sidebar-expanded-mobile {
-    left: 0;
-  }
-
   a {
     text-decoration: none;
   }
@@ -193,7 +186,9 @@
   }
 
   @include media-breakpoint-down(sm) {
-    left: (-$contextual-sidebar-width);
+    &.sidebar-collapsed {
+      left: (-$contextual-sidebar-width);
+    }
   }
 
   .nav-icon-container {
@@ -211,7 +206,7 @@
   }
 
   @media (min-width: map-get($grid-breakpoints, md)) and (max-width: map-get($grid-breakpoints, xl) - 1px) {
-    &:not(.sidebar-expanded-mobile) {
+    .sidebar-collapsed {
       @include collapse-contextual-sidebar;
       @include collapse-contextual-sidebar-content;
     }
@@ -396,7 +391,7 @@
   overflow: hidden;
 }
 
-.sidebar-collapsed-desktop {
+.sidebar-collapsed {
   @include collapse-contextual-sidebar-content;
 }
 
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index ffa5719fefb..32bed511195 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -260,10 +260,6 @@ module ApplicationHelper
     end
   end
 
-  def collapsed_sidebar?
-    cookies["sidebar_collapsed"] == "true"
-  end
-
   def locale_path
     asset_path("locale/#{Gitlab::I18n.locale}/app.js")
   end
diff --git a/app/helpers/contextual_sidebar_helper.rb b/app/helpers/contextual_sidebar_helper.rb
new file mode 100644
index 00000000000..1350bfd42f8
--- /dev/null
+++ b/app/helpers/contextual_sidebar_helper.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module ContextualSidebarHelper
+  def collapsed_sidebar?
+    cookies["sidebar_collapsed"] == "true"
+  end
+
+
+  def contextual_sidebar_css_class
+    collapsed_sidebar? ? collapsed_css_class : ''
+  end
+
+  private
+
+  def collapsed_css_class
+    'sidebar-collapsed js-sidebar-collapsed'
+  end
+end
diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml
index 2fdd65f639b..cf0e1d54046 100644
--- a/app/views/layouts/nav/sidebar/_admin.html.haml
+++ b/app/views/layouts/nav/sidebar/_admin.html.haml
@@ -1,4 +1,4 @@
-.nav-sidebar.qa-admin-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) }
+.nav-sidebar.qa-admin-sidebar{ class: contextual_sidebar_css_class }
   .nav-sidebar-inner-scroll
     .context-header
       = link_to admin_root_path, title: _('Admin Overview') do
diff --git a/app/views/layouts/nav/sidebar/_group.html.haml b/app/views/layouts/nav/sidebar/_group.html.haml
index 21ea9f3b2f3..01ec5e2d54a 100644
--- a/app/views/layouts/nav/sidebar/_group.html.haml
+++ b/app/views/layouts/nav/sidebar/_group.html.haml
@@ -2,7 +2,7 @@
 - merge_requests_count = group_merge_requests_count(state: 'opened')
 - issues_sub_menu_items = ['groups#issues', 'labels#index', 'milestones#index', 'boards#index', 'boards#show']
 
-.nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) }
+.nav-sidebar{ class: contextual_sidebar_css_class }
   .nav-sidebar-inner-scroll
     .context-header
       = link_to group_path(@group), title: @group.name do
diff --git a/app/views/layouts/nav/sidebar/_instance_statistics.html.haml b/app/views/layouts/nav/sidebar/_instance_statistics.html.haml
index 57180f27146..02a1e531e6f 100644
--- a/app/views/layouts/nav/sidebar/_instance_statistics.html.haml
+++ b/app/views/layouts/nav/sidebar/_instance_statistics.html.haml
@@ -1,4 +1,4 @@
-.nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) }
+.nav-sidebar{ class: contextual_sidebar_css_class }
   .nav-sidebar-inner-scroll
     .context-header
       = link_to instance_statistics_root_path, title: _('Instance Statistics') do
diff --git a/app/views/layouts/nav/sidebar/_profile.html.haml b/app/views/layouts/nav/sidebar/_profile.html.haml
index 1e3bb8f1224..7fc87e620c8 100644
--- a/app/views/layouts/nav/sidebar/_profile.html.haml
+++ b/app/views/layouts/nav/sidebar/_profile.html.haml
@@ -1,4 +1,4 @@
-.nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) }
+.nav-sidebar{ class: contextual_sidebar_css_class }
   .nav-sidebar-inner-scroll
     .context-header
       = link_to profile_path, title: _('Profile Settings') do
diff --git a/app/views/layouts/nav/sidebar/_project.html.haml b/app/views/layouts/nav/sidebar/_project.html.haml
index 7b492efeb09..d96aee1a70d 100644
--- a/app/views/layouts/nav/sidebar/_project.html.haml
+++ b/app/views/layouts/nav/sidebar/_project.html.haml
@@ -1,4 +1,4 @@
-.nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) }
+.nav-sidebar{ class: contextual_sidebar_css_class }
   .nav-sidebar-inner-scroll
     - can_edit = can?(current_user, :admin_project, @project)
     .context-header
diff --git a/spec/features/projects/user_sees_sidebar_spec.rb b/spec/features/projects/user_sees_sidebar_spec.rb
index 383e8824b7b..3ac147825b1 100644
--- a/spec/features/projects/user_sees_sidebar_spec.rb
+++ b/spec/features/projects/user_sees_sidebar_spec.rb
@@ -15,27 +15,23 @@ describe 'Projects > User sees sidebar' do
     shared_examples 'has a expanded nav sidebar' do
       it 'has a expanded desktop nav-sidebar on load' do
         expect(page).to have_content('Collapse sidebar')
-        expect(page).not_to have_selector('.sidebar-collapsed-desktop')
-        expect(page).not_to have_selector('.sidebar-expanded-mobile')
+        expect(page).not_to have_selector('.sidebar-collapsed')
       end
 
       it 'can collapse the nav-sidebar' do
         page.find('.nav-sidebar .js-toggle-sidebar').click
-        expect(page).to have_selector('.sidebar-collapsed-desktop')
+        expect(page).to have_selector('.sidebar-collapsed')
         expect(page).not_to have_content('Collapse sidebar')
-        expect(page).not_to have_selector('.sidebar-expanded-mobile')
       end
     end
 
     shared_examples 'has a collapsed nav sidebar' do
       it 'has a collapsed desktop nav-sidebar on load' do
         expect(page).not_to have_content('Collapse sidebar')
-        expect(page).not_to have_selector('.sidebar-expanded-mobile')
       end
 
       it 'can expand the nav-sidebar' do
         page.find('.nav-sidebar .js-toggle-sidebar').click
-        expect(page).to have_selector('.sidebar-expanded-mobile')
         expect(page).to have_content('Collapse sidebar')
       end
     end
@@ -43,13 +39,11 @@ describe 'Projects > User sees sidebar' do
     shared_examples 'has a mobile nav-sidebar' do
       it 'has a hidden nav-sidebar on load' do
         expect(page).not_to have_content('.mobile-nav-open')
-        expect(page).not_to have_selector('.sidebar-expanded-mobile')
       end
 
       it 'can expand the nav-sidebar' do
         page.find('.toggle-mobile-nav').click
         expect(page).to have_selector('.mobile-nav-open')
-        expect(page).to have_selector('.sidebar-expanded-mobile')
       end
     end
 
diff --git a/spec/javascripts/fly_out_nav_spec.js b/spec/javascripts/fly_out_nav_spec.js
index 7ef44f29c5b..b15c8d32c23 100644
--- a/spec/javascripts/fly_out_nav_spec.js
+++ b/spec/javascripts/fly_out_nav_spec.js
@@ -219,7 +219,7 @@ describe('Fly out sidebar navigation', () => {
     it('shows collapsed only sub-items if icon only sidebar', () => {
       const subItems = el.querySelector('.sidebar-sub-level-items');
       const sidebar = document.createElement('div');
-      sidebar.classList.add('sidebar-collapsed-desktop');
+      sidebar.classList.add('sidebar-collapsed');
       subItems.classList.add('is-fly-out-only');
 
       setSidebar(sidebar);
@@ -296,7 +296,7 @@ describe('Fly out sidebar navigation', () => {
 
     it('returns true when active & collapsed sidebar', () => {
       const sidebar = document.createElement('div');
-      sidebar.classList.add('sidebar-collapsed-desktop');
+      sidebar.classList.add('sidebar-collapsed');
       el.classList.add('active');
 
       setSidebar(sidebar);
diff --git a/spec/support/shared_examples/views/nav_sidebar.rb b/spec/support/shared_examples/views/nav_sidebar.rb
index 6ac5abe275d..3c52746b56f 100644
--- a/spec/support/shared_examples/views/nav_sidebar.rb
+++ b/spec/support/shared_examples/views/nav_sidebar.rb
@@ -5,7 +5,7 @@ shared_examples 'has nav sidebar' do
     render
 
     expect(rendered).to have_selector('.nav-sidebar')
-    expect(rendered).not_to have_selector('.sidebar-collapsed-desktop')
+    expect(rendered).not_to have_selector('.sidebar-collapsed')
     expect(rendered).not_to have_selector('.sidebar-expanded-mobile')
   end
 end