diff --git a/.changeset/navlist-parent-flicker-fix.md b/.changeset/navlist-parent-flicker-fix.md new file mode 100644 index 00000000000..8c0e46ee1f7 --- /dev/null +++ b/.changeset/navlist-parent-flicker-fix.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Fix NavList parent item flicker during static-to-interactive transitions when navigating between current sub-items in a SubNav. diff --git a/packages/react/src/NavList/NavList.dev.stories.tsx b/packages/react/src/NavList/NavList.dev.stories.tsx index d4316028b7f..18f94d5f47c 100644 --- a/packages/react/src/NavList/NavList.dev.stories.tsx +++ b/packages/react/src/NavList/NavList.dev.stories.tsx @@ -1,4 +1,6 @@ import type {Meta} from '@storybook/react-vite' +import React, {useEffect} from 'react' +import {renderToStaticMarkup} from 'react-dom/server' import {PageLayout} from '../PageLayout' import {NavList} from './NavList' import {ArrowRightIcon, ArrowLeftIcon, BookIcon, FileDirectoryIcon} from '@primer/octicons-react' @@ -69,3 +71,87 @@ export const WithGroupTitleAndHeading = () => ( ) + +type NavigationItem = 'item1' | 'subitem1' | 'subitem2' | 'item3' + +const NAVIGATION_FALLBACK_ITEM: NavigationItem = 'subitem1' + +function getNavigationItemFromHash(): NavigationItem { + const hash = window.location.hash.replace('#', '') + if (hash === 'item1' || hash === 'subitem1' || hash === 'subitem2' || hash === 'item3') { + return hash + } + return NAVIGATION_FALLBACK_ITEM +} + +function StaticTransitionNav({currentItem}: {currentItem: NavigationItem}) { + return ( + + + + + Item 1 + + + Item 2 + + + Sub item 1 + + + Sub item 2 + + + + + Item 3 + + + + + + ) +} + +export const SubNavStaticToInteractiveTransition = () => { + const [currentItem, setCurrentItem] = React.useState(() => getNavigationItemFromHash()) + const [renderMode, setRenderMode] = React.useState<'static' | 'interactive'>('static') + + const staticMarkup = React.useMemo(() => { + return renderToStaticMarkup() + }, [currentItem]) + + useEffect(() => { + const abortController = new AbortController() + let transitionTimer: number | null = null + + const showStaticThenInteractive = () => { + setCurrentItem(getNavigationItemFromHash()) + setRenderMode('static') + + if (transitionTimer) { + window.clearTimeout(transitionTimer) + } + + // Keep static markup visible long enough to make the collapsed frame obvious. + transitionTimer = window.setTimeout(() => { + setRenderMode('interactive') + }, 180) + } + + showStaticThenInteractive() + window.addEventListener('hashchange', showStaticThenInteractive, {signal: abortController.signal}) + + return () => { + abortController.abort() + if (transitionTimer) { + window.clearTimeout(transitionTimer) + } + } + }, []) + + if (renderMode === 'static') return
+ return +} + +SubNavStaticToInteractiveTransition.storyName = 'SubNav static-to-interactive transition' diff --git a/packages/react/src/NavList/NavList.test.tsx b/packages/react/src/NavList/NavList.test.tsx index 81e79482cef..7e103459442 100644 --- a/packages/react/src/NavList/NavList.test.tsx +++ b/packages/react/src/NavList/NavList.test.tsx @@ -1,6 +1,7 @@ import {describe, it, expect, vi} from 'vitest' import {render, fireEvent, act} from '@testing-library/react' import React from 'react' +import {renderToStaticMarkup} from 'react-dom/server' import {NavList} from './NavList' import {ReactRouterLikeLink} from '../Pagination/mocks/ReactRouterLink' import {implementsClassName} from '../utils/testing' @@ -148,6 +149,11 @@ describe('NavList.Item with NavList.SubNav', () => { expect(queryByRole('list', {name: 'Item 2'})).toBeNull() }) + it('renders parent item expanded on initial static render when SubNav contains the current item', () => { + const markup = renderToStaticMarkup() + expect(markup).toContain('aria-expanded="true"') + }) + it('hides SubNav by default if SubNav does not contain the current item', () => { const {queryByRole} = render() const subNav = queryByRole('list', {name: 'Item 2'}) diff --git a/packages/react/src/NavList/NavList.tsx b/packages/react/src/NavList/NavList.tsx index c00ef3813e0..75250f652b6 100644 --- a/packages/react/src/NavList/NavList.tsx +++ b/packages/react/src/NavList/NavList.tsx @@ -123,21 +123,47 @@ const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: s isOpen: false, }) +function hasCurrentNavItem(node: React.ReactNode): boolean { + if ( + !isValidElement<{ + children?: React.ReactNode + 'aria-current'?: NavListItemProps['aria-current'] + }>(node) + ) { + return false + } + + const ariaCurrent = node.props['aria-current'] + if (Boolean(ariaCurrent) && ariaCurrent !== 'false') { + return true + } + + if (!node.props.children) { + return false + } + + return React.Children.toArray(node.props.children).some(hasCurrentNavItem) +} + function ItemWithSubNav({children, subNav, depth: _depth, defaultOpen, style}: ItemWithSubNavProps) { const buttonId = useId() const subNavId = useId() - const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? false) - const subNavRef = React.useRef(null) - const [containsCurrentItem, setContainsCurrentItem] = React.useState(false) + + // We have to use recursion to check if the current nav item is part of the subnav before initial render + // which is why we can't use the querySelector on the ref as it will cause the parent item to blink during first render. + const hasCurrentItem = React.useMemo(() => hasCurrentNavItem(subNav), [subNav]) + const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? hasCurrentItem) + const subNavRef = React.useRef(null) + const [containsCurrentItem, setContainsCurrentItem] = React.useState(hasCurrentItem) useIsomorphicLayoutEffect(() => { if (subNavRef.current) { // Check if SubNav contains current item // valid values: page, step, location, date, time, true and false const currentItem = subNavRef.current.querySelector('[aria-current]:not([aria-current=false])') + setContainsCurrentItem(Boolean(currentItem)) if (currentItem) { - setContainsCurrentItem(true) setIsOpen(true) } }