Merge branch 'fly-out-nav-collapsed' into 'master'

Fix fly-out navigation not showing for active items in collapsed sidebar

See merge request !13353

3 files changed, 78 insertions, 9 deletions

diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js
index 301e82f4610..aabea56408a 100644
--- a/app/assets/javascripts/fly_out_nav.js
+++ b/app/assets/javascripts/fly_out_nav.js
@@ -1,7 +1,17 @@
 /* global bp */
+import Cookies from 'js-cookie';
 import './breakpoints';
 
-export const canShowSubItems = () => bp.getBreakpointSize() === 'md' || bp.getBreakpointSize() === 'lg';
+export const canShowActiveSubItems = (el) => {
+  const isHiddenByMedia = bp.getBreakpointSize() === 'sm' || bp.getBreakpointSize() === 'md';
+
+  if (el.classList.contains('active') && !isHiddenByMedia) {
+    return Cookies.get('sidebar_collapsed') === 'true';
+  }
+
+  return true;
+};
+export const canShowSubItems = () => bp.getBreakpointSize() === 'sm' || bp.getBreakpointSize() === 'md' || bp.getBreakpointSize() === 'lg';
 
 export const calculateTop = (boundingRect, outerHeight) => {
   const windowHeight = window.innerHeight;
@@ -14,9 +24,10 @@ export const calculateTop = (boundingRect, outerHeight) => {
 export const showSubLevelItems = (el) => {
   const subItems = el.querySelector('.sidebar-sub-level-items');
 
-  if (!subItems || !canShowSubItems()) return;
+  if (!subItems || !canShowSubItems() || !canShowActiveSubItems(el)) return;
 
   subItems.style.display = 'block';
+  el.classList.add('is-showing-fly-out');
   el.classList.add('is-over');
 
   const boundingRect = el.getBoundingClientRect();
@@ -34,15 +45,16 @@ export const showSubLevelItems = (el) => {
 export const hideSubLevelItems = (el) => {
   const subItems = el.querySelector('.sidebar-sub-level-items');
 
-  if (!subItems || !canShowSubItems()) return;
+  if (!subItems || !canShowSubItems() || !canShowActiveSubItems(el)) return;
 
+  el.classList.remove('is-showing-fly-out');
   el.classList.remove('is-over');
   subItems.style.display = 'none';
   subItems.classList.remove('is-above');
 };
 
 export default () => {
-  const items = [...document.querySelectorAll('.sidebar-top-level-items > li:not(.active)')]
+  const items = [...document.querySelectorAll('.sidebar-top-level-items > li')]
     .filter(el => el.querySelector('.sidebar-sub-level-items'));
 
   items.forEach((el) => {
diff --git a/app/assets/stylesheets/new_sidebar.scss b/app/assets/stylesheets/new_sidebar.scss
index 609bc9a7dfc..3fc8939f658 100644
--- a/app/assets/stylesheets/new_sidebar.scss
+++ b/app/assets/stylesheets/new_sidebar.scss
@@ -218,9 +218,8 @@ $new-sidebar-collapsed-width: 50px;
       }
     }
 
-    &:not(.active) {
+    &.is-showing-fly-out {
       > a {
-        margin-left: 1px;
         margin-right: 2px;
       }
 
@@ -271,6 +270,14 @@ $new-sidebar-collapsed-width: 50px;
             }
           }
 
+          > .active {
+            box-shadow: none;
+
+            > a {
+              background-color: transparent;
+            }
+          }
+
           a {
             padding: 8px 16px;
             color: $gl-text-color;
diff --git a/spec/javascripts/fly_out_nav_spec.js b/spec/javascripts/fly_out_nav_spec.js
index ab74f3e00ec..ea2a4caffaf 100644
--- a/spec/javascripts/fly_out_nav_spec.js
+++ b/spec/javascripts/fly_out_nav_spec.js
@@ -1,9 +1,11 @@
 /* global bp */
+import Cookies from 'js-cookie';
 import {
   calculateTop,
   hideSubLevelItems,
   showSubLevelItems,
   canShowSubItems,
+  canShowActiveSubItems,
 } from '~/fly_out_nav';
 
 describe('Fly out sidebar navigation', () => {
@@ -61,7 +63,7 @@ describe('Fly out sidebar navigation', () => {
     });
 
     it('does not hude subitems on mobile', () => {
-      breakpointSize = 'sm';
+      breakpointSize = 'xs';
 
       hideSubLevelItems(el);
 
@@ -121,7 +123,7 @@ describe('Fly out sidebar navigation', () => {
     });
 
     it('does not show sub-items on mobile', () => {
-      breakpointSize = 'sm';
+      breakpointSize = 'xs';
 
       showSubLevelItems(el);
 
@@ -170,11 +172,59 @@ describe('Fly out sidebar navigation', () => {
     });
 
     it('returns false if on mobile size', () => {
-      breakpointSize = 'sm';
+      breakpointSize = 'xs';
 
       expect(
         canShowSubItems(),
       ).toBeFalsy();
     });
   });
+
+  describe('canShowActiveSubItems', () => {
+    afterEach(() => {
+      Cookies.remove('sidebar_collapsed');
+    });
+
+    it('returns true by default', () => {
+      expect(
+        canShowActiveSubItems(el),
+      ).toBeTruthy();
+    });
+
+    it('returns false when cookie is false & element is active', () => {
+      Cookies.set('sidebar_collapsed', 'false');
+      el.classList.add('active');
+
+      expect(
+        canShowActiveSubItems(el),
+      ).toBeFalsy();
+    });
+
+    it('returns true when cookie is false & element is active', () => {
+      Cookies.set('sidebar_collapsed', 'true');
+      el.classList.add('active');
+
+      expect(
+        canShowActiveSubItems(el),
+      ).toBeTruthy();
+    });
+
+    it('returns true when element is active & breakpoint is sm', () => {
+      breakpointSize = 'sm';
+      el.classList.add('active');
+
+      expect(
+        canShowActiveSubItems(el),
+      ).toBeTruthy();
+    });
+
+    it('returns true when element is active & breakpoint is md', () => {
+      breakpointSize = 'md';
+      el.classList.add('active');
+
+      expect(
+        canShowActiveSubItems(el),
+      ).toBeTruthy();
+    });
+  });
 });