diff --git a/src/components/layout/sidebar-navigation/SidebarNavigation.tsx b/src/components/layout/sidebar-navigation/SidebarNavigation.tsx index 63b51ad2..3da4c224 100644 --- a/src/components/layout/sidebar-navigation/SidebarNavigation.tsx +++ b/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')}; } diff --git a/src/components/layout/sidebar-navigation/SidebarNavigationAccordionItem.tsx b/src/components/layout/sidebar-navigation/SidebarNavigationAccordionItem.tsx index cbd802b1..dd4dca59 100644 --- a/src/components/layout/sidebar-navigation/SidebarNavigationAccordionItem.tsx +++ b/src/components/layout/sidebar-navigation/SidebarNavigationAccordionItem.tsx @@ -163,7 +163,7 @@ 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')}; @@ -171,33 +171,40 @@ const AccordionItemPanel = styled.section` // 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'; diff --git a/src/components/layout/sidebar-navigation/SidebarNavigationBody.tsx b/src/components/layout/sidebar-navigation/SidebarNavigationBody.tsx index d9d619de..198d2467 100644 --- a/src/components/layout/sidebar-navigation/SidebarNavigationBody.tsx +++ b/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'; diff --git a/src/components/layout/sidebar-navigation/SidebarNavigationGroup.tsx b/src/components/layout/sidebar-navigation/SidebarNavigationGroup.tsx index 1b974fbe..53dbb045 100644 --- a/src/components/layout/sidebar-navigation/SidebarNavigationGroup.tsx +++ b/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}> - + {label} - + + + {children} @@ -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; diff --git a/src/components/layout/sidebar-navigation/SidebarNavigationItem.tsx b/src/components/layout/sidebar-navigation/SidebarNavigationItem.tsx index 27acd75e..6858d1e9 100644 --- a/src/components/layout/sidebar-navigation/SidebarNavigationItem.tsx +++ b/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')}; diff --git a/src/components/layout/sidebar-navigation/__tests__/SidebarNavigationAccordionItem-test.tsx b/src/components/layout/sidebar-navigation/__tests__/SidebarNavigationAccordionItem-test.tsx index 45471613..697e9eb0 100644 --- a/src/components/layout/sidebar-navigation/__tests__/SidebarNavigationAccordionItem-test.tsx +++ b/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)', + ); }); }); diff --git a/src/components/layout/sidebar-navigation/__tests__/SidebarNavigationItem-test.tsx b/src/components/layout/sidebar-navigation/__tests__/SidebarNavigationItem-test.tsx index 1261b42d..635552d9 100644 --- a/src/components/layout/sidebar-navigation/__tests__/SidebarNavigationItem-test.tsx +++ b/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();