ECHOES-1032 Align icons in sidebar collapsed mode (#599)

Co-authored-by: Grégoire Aubert <[email protected]>

Author: david-cho-lerat-sonarsource

src/components/layout/sidebar-navigation/SidebarNavigation.tsx

@@ -99,7 +99,7 @@ const SidebarNavigationWrapper = styled.nav`
 
   --sidebar-navigation-width: ${cssVar('layout-sidebar-navigation-sizes-width-open')};
 
-  // hover and focus-within pilotes the open state of the sidebar
+  // hover and focus-within pilots the open state of the sidebar
   [data-sidebar-docked='false'] &:not(:hover, :focus-within) {
     --sidebar-navigation-width: ${cssVar('layout-sidebar-navigation-sizes-width-closed')};
   }

src/components/layout/sidebar-navigation/SidebarNavigationAccordionItem.tsx

@@ -163,41 +163,48 @@ const AccordionItem = styled.button`
 AccordionItem.displayName = 'AccordionItem';
 
 const AccordionItemPanel = styled.section`
-  margin-left: ${cssVar('dimension-space-300')};
+  margin-left: ${cssVar('dimension-space-200')};
   padding-left: ${cssVar('dimension-space-100')};
   padding-right: ${cssVar('dimension-space-100')};
   border-left: ${cssVar('border-width-default')} solid ${cssVar('color-border-weak')};
 
   // The children SidebarNavigationItems rely on this css property to set their display value, falling
   // back to flex if not inside an accordion
   --sidebar-navigation-accordion-children-display: flex;
-  --sidebar-navigation-accordion-children-gap: ${cssVar('dimension-space-50')};
+  --sidebar-navigation-accordion-children-visibility: visible;
 
-  // The next two rules hide the child SidebarNavigationItems when the accordion is closed, and also
-  // when the sidebar is neither docked nor open (hovered or focused).
+  // The rule hide the child SidebarNavigationItems when the accordion is closed
   &[data-accordion-open='false'] {
     --sidebar-navigation-accordion-children-display: none;
-    --sidebar-navigation-accordion-children-gap: ${cssVar('dimension-space-0')};
   }
 
+  // This rule hides the SidebarNavigationItems when the accordion is open but the sidebar is collapsed
+  // using visibility to make sure it still takes space to avoid layout shift when hovering the sidebar
   [data-sidebar-docked='false'] nav:not(:hover, :focus-within) & {
-    margin: 0;
-    padding: 0;
-    border-left: none;
-
-    --sidebar-navigation-accordion-children-display: none;
-    --sidebar-navigation-accordion-children-gap: ${cssVar('dimension-space-0')};
+    --sidebar-navigation-accordion-children-visibility: hidden;
+    --sidebar-navigation-accordion-children-outline: ${cssVar('color-surface-default')} solid
+      ${cssVar('focus-border-width-default')};
   }
 `;
 
 AccordionItemPanel.displayName = 'AccordionItemPanel';
 
-export const AccordionItemsList = styled.ul`
+const AccordionItemsList = styled.ul`
   all: unset;
 
   display: flex;
   flex-direction: column;
-  gap: var(--sidebar-navigation-accordion-children-gap);
+  gap: ${cssVar('dimension-space-50')};
+
+  [data-accordion-open='false'] & {
+    // Override the gap to avoid extra space that shows the left border when the accordion is closed
+    gap: ${cssVar('dimension-space-0')};
+  }
+
+  [data-sidebar-docked='false'] nav:not(:hover, :focus-within) & {
+    margin-left: calc(-1 * ${cssVar('dimension-space-300')});
+    width: ${cssVar('dimension-width-400')};
+  }
 `;
 
 AccordionItemsList.displayName = 'AccordionItemsList';

src/components/layout/sidebar-navigation/SidebarNavigationBody.tsx

@@ -82,7 +82,7 @@ SidebarNavigationBodyInner.displayName = 'SidebarNavigationBodyInner';
 
 const SidebarNavigationBottomShadowScroll = styled(BottomShadowScroll)`
   [data-sidebar-docked='false'] nav:not(:hover, :focus-within) & {
-    display: none;
+    opacity: 0.5;
   }
 `;
 SidebarNavigationBottomShadowScroll.displayName = 'SidebarNavigationBottomShadowScroll';

src/components/layout/sidebar-navigation/SidebarNavigationGroup.tsx

@@ -22,6 +22,7 @@ import styled from '@emotion/styled';
 import { forwardRef, PropsWithChildren, useId } from 'react';
 import { TextNode } from '~types/utils';
 import { cssVar } from '~utils/design-tokens';
+import { Divider } from '../../divider';
 import { Text } from '../../typography';
 import { UnstyledListItem } from './SidebarNavigationItemStyles';
 
@@ -52,9 +53,11 @@ export const SidebarNavigationGroup = forwardRef<
         role="group"
         {...radixProps}>
         <SidebarNavigationGroupLabel id={id}>
-          <Text isSubtle size="small">
+          <SidebarNavigationGroupLabelText isSubtle size="small">
             {label}
-          </Text>
+          </SidebarNavigationGroupLabelText>
+
+          <SidebarNavigationGroupLabelDivider />
         </SidebarNavigationGroupLabel>
         <SidebarNavigationGroupList>{children}</SidebarNavigationGroupList>
       </SidebarNavigationGroupContainer>
@@ -87,12 +90,24 @@ const SidebarNavigationGroupLabel = styled.label`
   align-items: center;
   height: ${cssVar('dimension-height-800')};
   padding: 0 ${cssVar('dimension-space-100')};
+  white-space: nowrap;
+`;
+SidebarNavigationGroupLabel.displayName = 'SidebarNavigationGroupLabel';
 
+const SidebarNavigationGroupLabelText = styled(Text)`
   [data-sidebar-docked='false'] nav:not(:hover, :focus-within) & {
     display: none;
   }
 `;
-SidebarNavigationGroupLabel.displayName = 'SidebarNavigationGroupLabel';
+SidebarNavigationGroupLabelText.displayName = 'SidebarNavigationGroupLabelText';
+
+const SidebarNavigationGroupLabelDivider = styled(Divider)`
+  [data-sidebar-docked='true'] &,
+  [data-sidebar-docked='false'] nav:is(:hover, :focus-within) & {
+    display: none;
+  }
+`;
+SidebarNavigationGroupLabelDivider.displayName = 'SidebarNavigationGroupLabelDivider';
 
 export const SidebarNavigationGroupList = styled.ul`
   all: unset;

src/components/layout/sidebar-navigation/SidebarNavigationItem.tsx

@@ -135,15 +135,24 @@ SidebarNavigationItem.displayName = 'SidebarNavigationItem';
 const NavigationItem = styled(NavLinkBase)`
   ${sidebarNavigationBaseItemStyles}
 
-  // When the item is inside an accordion, the display value changes based on the state of the accordion, hiding the items when the sidebar is closed
-  // This css property is set by the SidebarNavigationAccordionItem component
-  // Fallback to flex if not inside an accordion
+  // When the item is inside an accordion, the display/visibility value changes based on the state
+  // of the accordion, hiding the items when the sidebar is closed.
+  // These css properties are set by the SidebarNavigationAccordionItem component
+  // Fallback to flex/visible if not inside an accordion
   display: var(--sidebar-navigation-accordion-children-display, flex);
+  visibility: var(--sidebar-navigation-accordion-children-visibility, visible);
+
+  // Outline provided by the accordion item when the sidebar is collapsed, it's only visible for the
+  // active element, but we don't put it inside the active rule to make sure it doesn't have higher
+  // specificity than the outline provided by the sidebarNavigationBaseItemStyles on focus.
+  // Also it doesn't matter if it's present when not active since non active items are not visible anyway.
+  outline: var(--sidebar-navigation-accordion-children-outline);
 
   &:active,
   &.active {
     // Always display the item when active even if behind a closed accordion, this overrides the previously set display value from the css property
     display: flex;
+    visibility: visible;
 
     background-color: ${cssVar('sidebar-navigation-item-colors-background-active')};
     color: ${cssVar('color-text-accent')};

src/components/layout/sidebar-navigation/__tests__/SidebarNavigationAccordionItem-test.tsx

@@ -80,7 +80,7 @@ describe('ellipsis behavior', () => {
 });
 
 describe('integration with SidebarNavigationItem', () => {
-  it('should have active class and rely on the css property passed set form the accordion', () => {
+  it('should set CSS custom properties and active class on children', () => {
     setupSidebarNavigationAccordionItem({
       children: (
         <>
@@ -94,17 +94,42 @@ describe('integration with SidebarNavigationItem', () => {
       ),
     });
 
-    expect(screen.getByRole('link', { name: 'Sub Item 1' })).toHaveStyleRule(
+    const subItem1 = screen.getByRole('link', { name: 'Sub Item 1' });
+    const subItem2 = screen.getByRole('link', { name: 'Sub Item 2' });
+
+    // Check active class
+    expect(subItem1).toHaveClass('active');
+    expect(subItem2).not.toHaveClass('active');
+
+    // Check that display CSS custom property is set
+    expect(subItem1).toHaveStyleRule(
       'display',
       'var(--sidebar-navigation-accordion-children-display, flex)',
     );
-    expect(screen.getByRole('link', { name: 'Sub Item 1' })).toHaveClass('active');
-
-    expect(screen.queryByRole('link', { name: 'Sub Item 2' })).toHaveStyleRule(
+    expect(subItem2).toHaveStyleRule(
       'display',
       'var(--sidebar-navigation-accordion-children-display, flex)',
     );
-    expect(screen.getByRole('link', { name: 'Sub Item 2' })).not.toHaveClass('active');
+
+    // Check that visibility CSS custom property is set
+    expect(subItem1).toHaveStyleRule(
+      'visibility',
+      'var(--sidebar-navigation-accordion-children-visibility, visible)',
+    );
+    expect(subItem2).toHaveStyleRule(
+      'visibility',
+      'var(--sidebar-navigation-accordion-children-visibility, visible)',
+    );
+
+    // Check that outline CSS custom property is set
+    expect(subItem1).toHaveStyleRule(
+      'outline',
+      'var(--sidebar-navigation-accordion-children-outline)',
+    );
+    expect(subItem2).toHaveStyleRule(
+      'outline',
+      'var(--sidebar-navigation-accordion-children-outline)',
+    );
   });
 });
 

src/components/layout/sidebar-navigation/__tests__/SidebarNavigationItem-test.tsx

@@ -18,11 +18,14 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
+import { matchers } from '@emotion/jest';
 import { screen } from '@testing-library/react';
 import { renderWithMemoryRouter } from '~common/helpers/test-utils';
 import { IconBranch, IconClock } from '../../../icons';
 import { SidebarNavigationItem, SidebarNavigationItemProps } from '../SidebarNavigationItem';
 
+expect.extend(matchers);
+
 it('should handle onClick events', async () => {
   const handleClick = jest.fn();
   const { user } = setupSidebarNavigationItem({ onClick: handleClick });
@@ -91,6 +94,29 @@ describe('active state behavior', () => {
   });
 });
 
+describe('CSS custom properties for accordion integration', () => {
+  it('should use CSS custom properties for display, visibility and outline', () => {
+    setupSidebarNavigationItem();
+
+    const link = screen.getByRole('link');
+
+    // Check that display CSS custom property is used with fallback
+    expect(link).toHaveStyleRule(
+      'display',
+      'var(--sidebar-navigation-accordion-children-display, flex)',
+    );
+
+    // Check that visibility CSS custom property is used with fallback
+    expect(link).toHaveStyleRule(
+      'visibility',
+      'var(--sidebar-navigation-accordion-children-visibility, visible)',
+    );
+
+    // Check that outline CSS custom property is used (no fallback needed)
+    expect(link).toHaveStyleRule('outline', 'var(--sidebar-navigation-accordion-children-outline)');
+  });
+});
+
 it("shouldn't have any a11y violation", async () => {
   const { container } = setupSidebarNavigationItem({ Icon: IconBranch });
   await expect(container).toHaveNoA11yViolations();