diff options
4 files changed, 79 insertions, 67 deletions
diff --git a/app/assets/javascripts/contextual_sidebar.js b/app/assets/javascripts/contextual_sidebar.js index 8e2ff314434..67fcdd082a2 100644 --- a/app/assets/javascripts/contextual_sidebar.js +++ b/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) { diff --git a/app/assets/stylesheets/framework/contextual_sidebar.scss b/app/assets/stylesheets/framework/contextual_sidebar.scss index 58adda3a40b..3238b01c6c0 100644 --- a/app/assets/stylesheets/framework/contextual_sidebar.scss +++ b/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; + } } } diff --git a/spec/features/projects/user_sees_sidebar_spec.rb b/spec/features/projects/user_sees_sidebar_spec.rb index 736a574e937..383e8824b7b 100644 --- a/spec/features/projects/user_sees_sidebar_spec.rb +++ b/spec/features/projects/user_sees_sidebar_spec.rb @@ -4,14 +4,30 @@ 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 + 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') @@ -24,22 +40,20 @@ describe 'Projects > User sees sidebar' do end end - shared_examples 'has a desktop 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') + 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 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') + 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 xs size' do + context 'with a extra small viewport' do before do resize_screen_xs visit project_path(project) @@ -47,46 +61,48 @@ describe 'Projects > User sees sidebar' do expect(page).to have_selector('.toggle-mobile-nav') end - it 'has a collapsed 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_behaves_like 'has a mobile nav-sidebar' + 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') + 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 sm size' do + context 'with medium size viewport' do before do - resize_screen_sm + resize_window(768, 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 size 1199px' do + 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 diff --git a/spec/support/features/reportable_note_shared_examples.rb b/spec/support/features/reportable_note_shared_examples.rb index 8cfce49da8a..89dfbf931d2 100644 --- a/spec/support/features/reportable_note_shared_examples.rb +++ b/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) |