Automatic merge of gitlab-org/gitlab-ce master

app/assets/javascripts/contextual_sidebar.js

@@ -4,6 +4,10 @@ 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 {
   constructor() {
     this.initDomElements();
@@ -26,44 +30,54 @@ export default class ContextualSidebar {
   bindEvents() {
     if (!this.$sidebar.length) return;
 
-    document.addEventListener('click', e => {
-      if (
-        !e.target.closest('.nav-sidebar') &&
-        (bp.getBreakpointSize() === 'sm' || bp.getBreakpointSize() === 'md')
-      ) {
-        this.toggleCollapsedSidebar(true, true);
-      }
-    });
     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));
   }
 
+  // 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 (bp.getBreakpointSize() !== 'lg') {
+    if (!ContextualSidebar.isDesktopBreakpoint()) {
       return;
     }
     Cookies.set('sidebar_collapsed', value, { expires: 365 * 10 });
   }
 
   toggleSidebarNav(show) {
-    this.$sidebar.toggleClass('sidebar-expanded-mobile', show);
-    this.$overlay.toggleClass('mobile-nav-open', show);
+    const breakpoint = bp.getBreakpointSize();
+    const dbp = ContextualSidebar.isDesktopBreakpoint();
+
+    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');
   }
 
   toggleCollapsedSidebar(collapsed, saveCookie) {
     const breakpoint = bp.getBreakpointSize();
+    const dbp = ContextualSidebar.isDesktopBreakpoint();
 
     if (this.$sidebar.length) {
       this.$sidebar.toggleClass('sidebar-collapsed-desktop', collapsed);
-      this.$page.toggleClass('page-with-icon-sidebar', breakpoint === 'sm' ? true : collapsed);
+      this.$sidebar.toggleClass('sidebar-expanded-mobile', !dbp ? !collapsed : false);
+      this.$page.toggleClass(
+        'page-with-icon-sidebar',
+        breakpoint === 'xs' || breakpoint === 'sm' ? true : collapsed,
+      );
     }
 
     if (saveCookie) {
@@ -84,13 +98,11 @@ export default class ContextualSidebar {
   render() {
     if (!this.$sidebar.length) return;
 
-    const breakpoint = bp.getBreakpointSize();
-
-    if (breakpoint === 'sm' || breakpoint === 'md') {
-      this.toggleCollapsedSidebar(true, false);
-    } else if (breakpoint === 'lg') {
+    if (!ContextualSidebar.isDesktopBreakpoint()) {
+      this.toggleSidebarNav(false);
+    } else {
       const collapse = parseBoolean(Cookies.get('sidebar_collapsed'));
-      this.toggleCollapsedSidebar(collapse, false);
+      this.toggleCollapsedSidebar(collapse, true);
     }
   }
 }

app/assets/stylesheets/framework/contextual_sidebar.scss

@@ -5,7 +5,7 @@
     padding-left: $contextual-sidebar-collapsed-width;
   }
 
-  @include media-breakpoint-up(lg) {
+  @include media-breakpoint-up(xl) {
     padding-left: $contextual-sidebar-width;
   }
 
@@ -15,7 +15,7 @@
 }
 
 .page-with-icon-sidebar {
-  @include media-breakpoint-up(sm) {
+  @include media-breakpoint-up(md) {
     padding-left: $contextual-sidebar-collapsed-width;
   }
 }
@@ -71,6 +71,44 @@
   }
 }
 
+@mixin collapse-contextual-sidebar-content {
+  .context-header {
+    height: 60px;
+    width: $contextual-sidebar-collapsed-width;
+
+    a {
+      padding: 10px 4px;
+    }
+  }
+
+  .sidebar-top-level-items > li {
+    .sidebar-sub-level-items {
+      &:not(.flyout-list) {
+        display: none;
+      }
+    }
+  }
+
+  .nav-icon-container {
+    margin-right: 0;
+  }
+
+  .toggle-sidebar-button {
+    padding: 16px;
+    width: $contextual-sidebar-collapsed-width - 1px;
+
+    .collapse-text,
+    .icon-angle-double-left {
+      display: none;
+    }
+
+    .icon-angle-double-right {
+      display: block;
+      margin: 0;
+    }
+  }
+}
+
 .nav-sidebar {
   transition: width $sidebar-transition-duration, left $sidebar-transition-duration;
   position: fixed;
@@ -89,7 +127,7 @@
     }
   }
 
-  &.sidebar-collapsed-desktop {
+  @mixin collapse-contextual-sidebar {
     width: $contextual-sidebar-collapsed-width;
 
     .nav-sidebar-inner-scroll {
@@ -115,6 +153,10 @@
     }
   }
 
+  &.sidebar-collapsed-desktop {
+    @include collapse-contextual-sidebar;
+  }
+
   &.sidebar-expanded-mobile {
     left: 0;
   }
@@ -150,7 +192,7 @@
     }
   }
 
-  @include media-breakpoint-down(xs) {
+  @include media-breakpoint-down(sm) {
     left: (-$contextual-sidebar-width);
   }
 
@@ -167,16 +209,19 @@
     height: 16px;
     width: 16px;
   }
+
+  @media (min-width: map-get($grid-breakpoints, md)) and (max-width: map-get($grid-breakpoints, xl) - 1px) {
+    &:not(.sidebar-expanded-mobile) {
+      @include collapse-contextual-sidebar;
+      @include collapse-contextual-sidebar-content;
+    }
+  }
 }
 
 .nav-sidebar-inner-scroll {
   height: 100%;
   width: 100%;
   overflow: auto;
-
-  @include media-breakpoint-up(sm) {
-    overflow: hidden;
-  }
 }
 
 .with-performance-bar .nav-sidebar {
@@ -346,53 +391,13 @@
   }
 }
 
-.toggle-sidebar-button {
-  @include media-breakpoint-down(xs) {
-    display: none;
-  }
-}
-
 .collapse-text {
   white-space: nowrap;
   overflow: hidden;
 }
 
 .sidebar-collapsed-desktop {
-  .context-header {
-    height: 60px;
-    width: $contextual-sidebar-collapsed-width;
-
-    a {
-      padding: 10px 4px;
-    }
-  }
-
-  .sidebar-top-level-items > li {
-    .sidebar-sub-level-items {
-      &:not(.flyout-list) {
-        display: none;
-      }
-    }
-  }
-
-  .nav-icon-container {
-    margin-right: 0;
-  }
-
-  .toggle-sidebar-button {
-    padding: 16px;
-    width: $contextual-sidebar-collapsed-width - 1px;
-
-    .collapse-text,
-    .icon-angle-double-left {
-      display: none;
-    }
-
-    .icon-angle-double-right {
-      display: block;
-      margin: 0;
-    }
-  }
+  @include collapse-contextual-sidebar-content;
 }
 
 .fly-out-top-item {
@@ -428,16 +433,14 @@
     color: $gl-text-color-secondary;
   }
 
-  @include media-breakpoint-down(xs) {
+  @include media-breakpoint-down(sm) {
     display: flex;
     align-items: center;
 
     i {
       font-size: 18px;
     }
-  }
 
-  @include media-breakpoint-down(xs) {
     + .breadcrumbs-links {
       padding-left: $gl-padding;
       border-left: 1px solid $gl-text-color-quaternary;
@@ -445,13 +448,16 @@
   }
 }
 
-@include media-breakpoint-down(xs) {
+@include media-breakpoint-down(sm) {
   .close-nav-button {
     display: flex;
   }
-}
 
-.mobile-overlay {
+  .toggle-sidebar-button {
+    display: none;
+  }
+
+  .mobile-overlay {
     display: none;
 
     &.mobile-nav-open {
@@ -462,4 +468,5 @@
       width: 100%;
       z-index: 300;
     }
+  }
 }

changelogs/unreleased/fix-38010-sidebar-loads-and-collapses.yml

+---
+title: Fixed navigation sidebar flashing open on page load 
+merge_request: 24555
+author:
+type: fixed

spec/features/projects/user_sees_sidebar_spec.rb

@@ -4,6 +4,108 @@ describe 'Projects > User sees sidebar' do
   let(:user) { create(:user) }
   let(:project) { create(:project, :private, public_builds: false, namespace: user.namespace) }
 
+  # NOTE: See documented behaviour https://design.gitlab.com/regions/navigation#contextual-navigation
+  context 'on different viewports', :js do
+    include MobileHelpers
+
+    before do
+      sign_in(user)
+    end
+
+    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')
+      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).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
+
+    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
+
+    context 'with a extra small viewport' do
+      before do
+        resize_screen_xs
+        visit project_path(project)
+        expect(page).to have_selector('.nav-sidebar')
+        expect(page).to have_selector('.toggle-mobile-nav')
+      end
+
+      it_behaves_like 'has a mobile nav-sidebar'
+    end
+
+    context 'with a small size viewport' do
+      before do
+        resize_screen_sm
+        visit project_path(project)
+        expect(page).to have_selector('.nav-sidebar')
+        expect(page).to have_selector('.toggle-mobile-nav')
+      end
+
+      it_behaves_like 'has a mobile nav-sidebar'
+    end
+
+    context 'with medium size viewport' do
+      before do
+        resize_window(768, 800)
+        visit project_path(project)
+        expect(page).to have_selector('.nav-sidebar')
+      end
+
+      it_behaves_like 'has a collapsed nav sidebar'
+    end
+
+    context 'with viewport size 1199px' do
+      before do
+        resize_window(1199, 800)
+        visit project_path(project)
+        expect(page).to have_selector('.nav-sidebar')
+      end
+
+      it_behaves_like 'has a collapsed nav sidebar'
+    end
+
+    context 'with a extra large viewport' do
+      before do
+        resize_window(1200, 800)
+        visit project_path(project)
+        expect(page).to have_selector('.nav-sidebar')
+      end
+
+      it_behaves_like 'has a expanded nav sidebar'
+    end
+  end
+
   context 'as owner' do
     before do
       sign_in(user)

spec/support/features/reportable_note_shared_examples.rb

@@ -41,7 +41,7 @@ shared_examples 'reportable note' do |type|
 
   def open_dropdown(dropdown)
     # make window wide enough that tooltip doesn't trigger horizontal scrollbar
-    resize_window(1200, 800)
+    restore_window_size
 
     dropdown.find('.more-actions-toggle').click
     dropdown.find('.dropdown-menu li', match: :first)

spec/support/shared_examples/views/nav_sidebar.rb

+# frozen_string_literal: true
+
+shared_examples 'has nav sidebar' do
+  it 'has collapsed nav sidebar on mobile' 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-expanded-mobile')
+  end
+end

spec/views/layouts/nav/sidebar/_admin.html.haml_spec.rb

@@ -26,6 +26,8 @@ describe 'layouts/nav/sidebar/_admin' do
     it_behaves_like 'page has active tab', 'Overview'
   end
 
+  it_behaves_like 'has nav sidebar'
+
   context 'on projects' do
     before do
       allow(controller).to receive(:controller_name).and_return('projects')

spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'layouts/nav/sidebar/_group' do
+  let(:group) { create(:group) }
+
+  before do
+    assign(:group, group)
+  end
+
+  it_behaves_like 'has nav sidebar'
+end

spec/views/layouts/nav/sidebar/_instance_statistics.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'layouts/nav/sidebar/_instance_statistics' do
+  it_behaves_like 'has nav sidebar'
+end

spec/views/layouts/nav/sidebar/_profile.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'layouts/nav/sidebar/_profile' do
+  let(:user) { create(:user) }
+
+  before do
+    allow(view).to receive(:current_user).and_return(user)
+  end
+
+  it_behaves_like 'has nav sidebar'
+end

spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb

@@ -11,6 +11,8 @@ describe 'layouts/nav/sidebar/_project' do
     allow(view).to receive(:can?).and_return(true)
   end
 
+  it_behaves_like 'has nav sidebar'
+
   describe 'issue boards' do
     it 'has board tab' do
       render