Align nav-sidebar with design.gitlab.com docs

Updated nav-sidebar to reflect the documented
behaviour from the design.gitlab.com docs:
https://design.gitlab.com/regions/navigation#contextual-navigation

Updated rspec tests to reflect the expected behaviour

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();
@@ -41,13 +45,9 @@ export default class ContextualSidebar {
     $(window).on('resize', () => _.debounce(this.render(), 100));
   }
 
-  // NOTE: at 1200px nav sidebar should be in 'desktop mode' (not overlap the content)
-  // https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24555#note_134136110
-  // But setting 'desktop mode' at 1200px will break spec/support/features/reportable_note_shared_examples.rb
-
   // TODO: use the breakpoints from breakpoints.js once they have been updated for bootstrap 4
-  // See related issue and discussion: https://gitlab.com/gitlab-org/gitlab-ce/issues/56745
-  static isDesktopBreakpoint = () => bp.windowWidth() >= 1200;
+  // See documentation: https://design.gitlab.com/regions/navigation#contextual-navigation
+  static isDesktopBreakpoint = () => bp.windowWidth() >= NAV_SIDEBAR_BREAKPOINT;
   static setCollapsedCookie(value) {
     if (!ContextualSidebar.isDesktopBreakpoint()) {
       return;
@@ -60,18 +60,24 @@ export default class ContextualSidebar {
     const dbp = ContextualSidebar.isDesktopBreakpoint();
 
     this.$sidebar.toggleClass('sidebar-expanded-mobile', !dbp ? show : false);
-    this.$overlay.toggleClass('mobile-nav-open', breakpoint === 'xs' ? 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(breakpoint);
+    const dbp = ContextualSidebar.isDesktopBreakpoint();
 
     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 === 'sm' ? true : collapsed);
+      this.$page.toggleClass(
+        'page-with-icon-sidebar',
+        breakpoint === 'xs' || breakpoint === 'sm' ? true : collapsed,
+      );
     }
 
     if (saveCookie) {

app/assets/stylesheets/framework/contextual_sidebar.scss

@@ -1,12 +1,10 @@
 .page-with-contextual-sidebar {
   transition: padding-left $sidebar-transition-duration;
 
-  @include media-breakpoint-up(sm) {
+  @include media-breakpoint-up(md) {
     padding-left: $contextual-sidebar-collapsed-width;
   }
 
-  // At 1200px nav sidebar should not overlap the content
-  // https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24555#note_134136110
   @include media-breakpoint-up(xl) {
     padding-left: $contextual-sidebar-width;
   }
@@ -17,7 +15,7 @@
 }
 
 .page-with-icon-sidebar {
-  @include media-breakpoint-up(sm) {
+  @include media-breakpoint-up(md) {
     padding-left: $contextual-sidebar-collapsed-width;
   }
 }
@@ -194,7 +192,7 @@
     }
   }
 
-  @include media-breakpoint-down(xs) {
+  @include media-breakpoint-down(sm) {
     left: (-$contextual-sidebar-width);
   }
 
@@ -212,7 +210,7 @@
     width: 16px;
   }
 
-  @media (min-width: map-get($grid-breakpoints, sm)) and (max-width: map-get($grid-breakpoints, xl) - 1px) {
+  @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;
@@ -224,10 +222,6 @@
   height: 100%;
   width: 100%;
   overflow: auto;
-
-  @include media-breakpoint-up(sm) {
-    overflow: hidden;
-  }
 }
 
 .with-performance-bar .nav-sidebar {
@@ -397,12 +391,6 @@
   }
 }
 
-.toggle-sidebar-button {
-  @include media-breakpoint-down(xs) {
-    display: none;
-  }
-}
-
 .collapse-text {
   white-space: nowrap;
   overflow: hidden;
@@ -445,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;
@@ -462,21 +448,25 @@
   }
 }
 
-@include media-breakpoint-down(xs) {
+@include media-breakpoint-down(sm) {
   .close-nav-button {
     display: flex;
   }
-}
 
-.mobile-overlay {
-  display: none;
+  .toggle-sidebar-button {
+    display: none;
+  }
 
-  &.mobile-nav-open {
-    display: block;
-    position: fixed;
-    background-color: $black-transparent;
-    height: 100%;
-    width: 100%;
-    z-index: 300;
+  .mobile-overlay {
+    display: none;
+
+    &.mobile-nav-open {
+      display: block;
+      position: fixed;
+      background-color: $black-transparent;
+      height: 100%;
+      width: 100%;
+      z-index: 300;
+    }
   }
 }

spec/features/projects/user_sees_sidebar_spec.rb

@@ -4,27 +4,15 @@ describe 'Projects > User sees sidebar' do
   let(:user) { create(:user) }
   let(:project) { create(:project, :private, public_builds: false, namespace: user.namespace) }
 
-  context 'on a smaller screen', :js do
+  # 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 collapsible mobile 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 desktop nav 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')
@@ -39,15 +27,21 @@ describe 'Projects > User sees sidebar' do
       end
     end
 
-    context 'with xs size' 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')
+    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 'has a collapsed nav-sidebar on load' do
+      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
@@ -59,34 +53,56 @@ describe 'Projects > User sees sidebar' do
       end
     end
 
-    context 'with sm size' do
+    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 collapsible mobile nav sidebar'
+      it_behaves_like 'has a mobile nav-sidebar'
     end
 
-    context 'with size 1199px' do
+    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 collapsible mobile nav sidebar'
+      it_behaves_like 'has a collapsed nav sidebar'
     end
 
-    context 'with a larger screen' do
+    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 desktop nav sidebar'
+      it_behaves_like 'has a expanded nav sidebar'
     end
   end
 

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)