diff --git a/.changeset/underline-nav-css-overflow.md b/.changeset/underline-nav-css-overflow.md new file mode 100644 index 00000000000..6fef9b316fa --- /dev/null +++ b/.changeset/underline-nav-css-overflow.md @@ -0,0 +1,5 @@ +--- +"@primer/react": minor +--- + +Removes `UnderlineNav` dynamic overflow functionality, replacing with simple scroll container to simplify logic, improve performance, and remove layout shifts, especially when using server-side rendering. diff --git a/e2e/components/UnderlineNav.test.ts b/e2e/components/UnderlineNav.test.ts index dc8546fa4cc..50c2d6cc474 100644 --- a/e2e/components/UnderlineNav.test.ts +++ b/e2e/components/UnderlineNav.test.ts @@ -199,19 +199,19 @@ test.describe('UnderlineNav', () => { }) // Default state - // expect(await page.screenshot()).toMatchSnapshot() + expect(await page.screenshot()).toMatchSnapshot() await page.setViewportSize({width: viewports['primer.breakpoint.sm'], height: 768}) - await page.locator('button', {hasText: 'More Repository Items'}).waitFor() + await page.locator('button', {hasText: 'More items'}).waitFor() // Resize - // expect(await page.screenshot()).toMatchSnapshot() + expect(await page.screenshot()).toMatchSnapshot() - await page.getByRole('button', {name: 'More Repository Items'}).click() - // expect(await page.screenshot()).toMatchSnapshot() + await page.getByRole('button', {name: 'More items'}).click() + expect(await page.screenshot()).toMatchSnapshot() - await page.getByRole('link', {name: 'Settings (10)'}).click() - // expect(await page.screenshot()).toMatchSnapshot() + await page.getByRole('menuitem', {name: 'Settings (10)'}).click() + expect(await page.screenshot()).toMatchSnapshot() }) test('Hide icons when there is not enough space to display all list items @vrt', async ({page}) => { @@ -223,61 +223,13 @@ test.describe('UnderlineNav', () => { }) // Default State - // expect(await page.screenshot()).toMatchSnapshot() + expect(await page.screenshot()).toMatchSnapshot() // Resize await page.setViewportSize({width: viewports['primer.breakpoint.md'], height: 768}) // Icons should be hidden - // expect(await page.screenshot()).toMatchSnapshot() - }) - - test('Keep selected item visible @vrt', async ({page}) => { - await visit(page, { - id: 'components-underlinenav-features--overflow-template', - globals: { - colorScheme: theme, - }, - }) - await page.setViewportSize({width: viewports['primer.breakpoint.sm'], height: 768}) - - await page.locator('button', {hasText: 'More Repository Items'}).waitFor() - await page.getByRole('button', {name: 'More Repository Items'}).click() - await page.getByRole('link', {name: 'Settings (10)'}).click() - - // State after selecting the second last item - // expect(await page.screenshot()).toMatchSnapshot() - - // Resize - await page.setViewportSize({ - width: 1100, - height: 480, - }) - await page.locator('button', {hasText: 'More Repository Items'}).waitFor({ - state: 'hidden', - }) - - // Current state - // expect(await page.screenshot()).toMatchSnapshot() - - // Resize - await page.setViewportSize({ - width: 800, - height: 480, - }) - await page.locator('button', {hasText: 'More Repository Items'}).waitFor() - - // Current state - // expect(await page.screenshot()).toMatchSnapshot() - - // Resize - await page.setViewportSize({ - width: 600, - height: 480, - }) - await page.locator('button', {hasText: 'More Repository Items'}).waitFor() - // Current state - // expect(await page.screenshot()).toMatchSnapshot() + expect(await page.screenshot()).toMatchSnapshot() }) }) } diff --git a/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx b/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx index 6b35a2c739b..db7f49351b2 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx @@ -77,11 +77,7 @@ const items: {navigation: string; icon: React.ReactElement; counter?: number | s export const OverflowTemplate = ({initialSelectedIndex = 1}: {initialSelectedIndex?: number}) => { const [selectedIndex, setSelectedIndex] = React.useState(initialSelectedIndex) return ( - + {items.map((item, index) => ( { - return + return } OverflowOnNarrowScreen.parameters = { diff --git a/packages/react/src/UnderlineNav/UnderlineNav.interactions.stories.tsx b/packages/react/src/UnderlineNav/UnderlineNav.interactions.stories.tsx index 409737b58d3..942d92e1638 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.interactions.stories.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.interactions.stories.tsx @@ -110,7 +110,7 @@ SelectAMenuItem.play = async ({canvasElement}: {canvasElement: HTMLElement}) => await delay(1000) - const moreBtn = canvas.getByRole('button', {name: 'More Repository items'}) + const moreBtn = canvas.getByRole('button', {name: 'More items'}) userEvent.hover(moreBtn) await delay() @@ -131,35 +131,4 @@ SelectAMenuItem.play = async ({canvasElement}: {canvasElement: HTMLElement}) => expect(lastListItem).toEqual(menuListItem) } -const KeepSelectedItemVisible = () => { - return -} -// eslint-disable-next-line @typescript-eslint/ban-ts-comment -// @ts-ignore -KeepSelectedItemVisible.play = async ({canvasElement}: {canvasElement: HTMLElement}) => { - const canvas = within(canvasElement) - // await delay(2000) - const selectedItem = canvas.getByRole('link', {name: 'Settings (10)'}) - expect(selectedItem).toHaveAttribute('aria-current', 'page') - // change viewport - canvasElement.style.width = '900px' - await delay(1000) - expect(selectedItem).toHaveAttribute('aria-current', 'page') - canvasElement.style.width = '800px' - await delay(1000) - expect(selectedItem).toHaveAttribute('aria-current', 'page') - canvasElement.style.width = '700px' - await delay(1000) - expect(selectedItem).toHaveAttribute('aria-current', 'page') - canvasElement.style.width = '600px' - await delay(1000) - expect(selectedItem).toHaveAttribute('aria-current', 'page') - canvasElement.style.width = '500px' - await delay(1000) - const lastListItem = canvas.getByRole('list').children[2].children[0] - const menuListItem = canvas.getByRole('link', {name: 'Settings (10)'}) - // expect Settings be the last element on the list. - expect(lastListItem).toEqual(menuListItem) -} - -export {KeyboardNavigation, SelectAMenuItem, KeepSelectedItemVisible} +export {KeyboardNavigation, SelectAMenuItem} diff --git a/packages/react/src/UnderlineNav/UnderlineNav.module.css b/packages/react/src/UnderlineNav/UnderlineNav.module.css index d905aff3fcf..a6713fbb6b8 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.module.css +++ b/packages/react/src/UnderlineNav/UnderlineNav.module.css @@ -1,22 +1,37 @@ -.MenuItemContent { - display: flex; - align-items: center; - justify-content: space-between; -} +.ScrollContainer { + overflow-x: auto; + overflow-y: hidden; + -webkit-overflow-scrolling: auto; + scrollbar-width: thin; + scrollbar-color: var(--borderColor-muted) transparent; + scrollbar-gutter: stable; + + /* Progressive enhancement: Detect overflow using scroll-based animations. + This lets us calculate the icon visibility during SSR. */ + animation: detect-overflow linear; + animation-timeline: scroll(self inline); + + --UnderlineNav_icons-display: inline; + + [data-component='icon'] { + display: var(--UnderlineNav_icons-display); + } + + /* Even on browsers that support scroll-driven animations, we still need to + force icons to remain hidden with JS to avoid flickering as they cause/remove + overflow. Once they are hidden once, they remain hidden for the life of the + component. */ + &[data-hide-icons='true'] { + animation: none; + animation-timeline: none; -/* More button styles migrated from styles.ts (was moreBtnStyles) */ -.MoreButton { - margin: 0; /* reset Safari extra margin */ - border: 0; - background: transparent; - font-weight: var(--base-text-weight-normal); - box-shadow: none; - padding-top: var(--base-size-4); - padding-bottom: var(--base-size-4); - padding-left: var(--base-size-8); - padding-right: var(--base-size-8); + --UnderlineNav_icons-display: none; + } +} - & > [data-component='trailingVisual'] { - margin-left: 0; +@keyframes detect-overflow { + 0%, + 100% { + --UnderlineNav_icons-display: none; } } diff --git a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx index 6039ec2ad8a..3d7ec501109 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx @@ -1,6 +1,6 @@ import {describe, expect, it, vi} from 'vitest' import type React from 'react' -import {render, screen} from '@testing-library/react' +import {render, screen, within} from '@testing-library/react' import userEvent from '@testing-library/user-event' import { CodeIcon, @@ -16,6 +16,7 @@ import {UnderlineNav} from '.' import {implementsClassName} from '../utils/testing' import classes from '../internal/components/UnderlineTabbedInterface.module.css' import {clsx} from 'clsx' +import {page} from 'vitest/browser' const ResponsiveUnderlineNav = ({ selectedItemText = 'Code', @@ -78,7 +79,8 @@ describe('UnderlineNav', () => { it('renders icons correctly', () => { const {getByRole} = render() const nav = getByRole('navigation') - expect(nav.getElementsByTagName('svg').length).toEqual(7) + const list = within(nav).getByRole('list') + expect(list.getElementsByTagName('svg').length).toEqual(7) }) it('fires onSelect on click', async () => { @@ -141,9 +143,10 @@ describe('UnderlineNav', () => { expect(counter.textContent).toBe('\u00A0(120)') }) - it('respects loadingCounters prop', () => { + it('respects loadingCounters prop', async () => { + await page.viewport(1000, 500) const {getByRole} = render() - const item = getByRole('link', {name: 'Actions'}) + const item = getByRole('link', {name: 'Actions', hidden: true}) const loadingCounter = item.getElementsByTagName('span')[2] expect(loadingCounter.className).toContain('LoadingCounter') expect(loadingCounter.textContent).toBe('') diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index 14c52ee9683..75946948fb4 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -1,22 +1,9 @@ -import type {RefObject} from 'react' -import React, {useRef, forwardRef, useCallback, useState, useEffect} from 'react' -import {UnderlineNavContext} from './UnderlineNavContext' -import type {ResizeObserverEntry} from '../hooks/useResizeObserver' -import {useResizeObserver} from '../hooks/useResizeObserver' -import type {ChildWidthArray, ResponsiveProps, ChildSize} from './types' +import React, {forwardRef, useEffect, useRef, useState} from 'react' import VisuallyHidden from '../_VisuallyHidden' -import {dividerStyles, menuItemStyles, baseMenuMinWidth} from './styles' -import {UnderlineItemList, UnderlineWrapper, LoadingCounter, GAP} from '../internal/components/UnderlineTabbedInterface' -import {Button} from '../Button' -import {TriangleDownIcon} from '@primer/octicons-react' -import {useOnEscapePress} from '../hooks/useOnEscapePress' -import {useOnOutsideClick} from '../hooks/useOnOutsideClick' -import {useId} from '../hooks/useId' -import {ActionList} from '../ActionList' -import CounterLabel from '../CounterLabel' +import {UnderlineItemList, UnderlineWrapper} from '../internal/components/UnderlineTabbedInterface' import {invariant} from '../utils/invariant' import classes from './UnderlineNav.module.css' -import {getAnchoredPosition} from '@primer/behaviors' +import {UnderlineNavContext} from './UnderlineNavContext' export type UnderlineNavProps = { children: React.ReactNode @@ -34,116 +21,13 @@ export type UnderlineNavProps = { */ variant?: 'inset' | 'flush' } -// When page is loaded, we don't have ref for the more button as it is not on the DOM yet. -// However, we need to calculate number of possible items when the more button present as well. So using the width of the more button as a constant. -export const MORE_BTN_WIDTH = 86 -// The height is needed to make sure we don't have a layout shift when the more button is the only item in the nav. -const MORE_BTN_HEIGHT = 45 -const overflowEffect = ( - navWidth: number, - moreMenuWidth: number, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - childArray: Array>, - childWidthArray: ChildWidthArray, - noIconChildWidthArray: ChildWidthArray, - updateListAndMenu: (props: ResponsiveProps, iconsVisible: boolean, overflowMeasured: boolean) => void, -) => { - let iconsVisible = true - if (childWidthArray.length === 0) { - updateListAndMenu({items: childArray, menuItems: []}, iconsVisible, false) - return - } - const numberOfItemsPossible = calculatePossibleItems(childWidthArray, navWidth) - const numberOfItemsWithoutIconPossible = calculatePossibleItems(noIconChildWidthArray, navWidth) - // We need to take more menu width into account when calculating the number of items possible - const numberOfItemsPossibleWithMoreMenu = calculatePossibleItems( - noIconChildWidthArray, - navWidth, - moreMenuWidth || MORE_BTN_WIDTH, - ) - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const items: Array> = [] - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const menuItems: Array> = [] - - // First, we check if we can fit all the items with their icons - if (childArray.length <= numberOfItemsPossible) { - items.push(...childArray) - } else if (childArray.length <= numberOfItemsWithoutIconPossible) { - // if we can't fit all the items with their icons, we check if we can fit all the items without their icons - iconsVisible = false - items.push(...childArray) - } else { - // if we can't fit all the items without their icons, we keep the icons hidden and show the ones that doesn't fit into the list in the overflow menu - iconsVisible = false - - /* Below is an accessibility requirement. Never show only one item in the overflow menu. - * If there is only one item left to display in the overflow menu according to the calculation, - * we need to pull another item from the list into the overflow menu. - */ - const numberOfItemsInMenu = childArray.length - numberOfItemsPossibleWithMoreMenu - const numberOfListItems = - numberOfItemsInMenu === 1 ? numberOfItemsPossibleWithMoreMenu - 1 : numberOfItemsPossibleWithMoreMenu - for (const [index, child] of childArray.entries()) { - if (index < numberOfListItems) { - items.push(child) - } else { - const ariaCurrent = child.props['aria-current'] - const isCurrent = Boolean(ariaCurrent) && ariaCurrent !== 'false' - // We need to make sure to keep the selected item always visible. - // To do that, we swap the selected item with the last item in the list to make it visible. (When there is at least 1 item in the list to swap.) - if (isCurrent && numberOfListItems > 0) { - // If selected item couldn't make in to the list, we swap it with the last item in the list. - const indexToReplaceAt = numberOfListItems - 1 // because we are replacing the last item in the list - // splice method modifies the array by removing 1 item here at the given index and replace it with the "child" element then returns the removed item. - const propsectiveAction = items.splice(indexToReplaceAt, 1, child)[0] - menuItems.push(propsectiveAction) - } else { - menuItems.push(child) - } - } - } - } - updateListAndMenu({items, menuItems}, iconsVisible, true) -} - -export const getValidChildren = (children: React.ReactNode) => { +const getValidChildren = (children: React.ReactNode) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any return React.Children.toArray(children).filter(child => React.isValidElement(child)) as React.ReactElement[] } -const calculatePossibleItems = (childWidthArray: ChildWidthArray, navWidth: number, moreMenuWidth = 0) => { - const widthToFit = navWidth - moreMenuWidth - let breakpoint = childWidthArray.length // assume all items will fit - let sumsOfChildWidth = 0 - for (const [index, childWidth] of childWidthArray.entries()) { - sumsOfChildWidth = sumsOfChildWidth + childWidth.width + GAP - if (sumsOfChildWidth > widthToFit) { - breakpoint = index - break - } else { - continue - } - } - return breakpoint -} - -// Inline styles converted from baseMenuStyles for use as CSSProperties -const baseMenuInlineStyles: React.CSSProperties = { - position: 'absolute', - zIndex: 1, - top: '90%', - boxShadow: '0 1px 3px rgba(0,0,0,0.12), 0 1px 2px rgba(0,0,0,0.24)', - borderRadius: 12, - background: 'var(--overlay-bgColor)', - listStyle: 'none', - minWidth: `${baseMenuMinWidth}px`, - maxWidth: '640px', - right: 0, -} - -export const UnderlineNav = forwardRef( +export const UnderlineNav = forwardRef( ( { as = 'nav', @@ -155,42 +39,9 @@ export const UnderlineNav = forwardRef( }: UnderlineNavProps, forwardedRef, ) => { - const backupRef = useRef(null) - const navRef = (forwardedRef ?? backupRef) as RefObject - const listRef = useRef(null) - const moreMenuRef = useRef(null) - const moreMenuBtnRef = useRef(null) - const containerRef = React.useRef(null) - const disclosureWidgetId = useId() - - const [isWidgetOpen, setIsWidgetOpen] = useState(false) - const [iconsVisible, setIconsVisible] = useState(true) - const [childWidthArray, setChildWidthArray] = useState([]) - const [noIconChildWidthArray, setNoIconChildWidthArray] = useState([]) - // Track whether the initial overflow calculation is complete to prevent CLS - const [isOverflowMeasured, setIsOverflowMeasured] = useState(false) - - const validChildren = getValidChildren(children) - - // Responsive props object manages which items are in the list and which items are in the menu. - const [responsiveProps, setResponsiveProps] = useState({ - items: validChildren, - menuItems: [], - }) - - // Make sure to have the fresh props data for list items when children are changed (keeping aria-current up-to-date) - const listItems = responsiveProps.items.map(item => { - return validChildren.find(child => child.key === item.key) ?? item - }) - - // Make sure to have the fresh props data for menu items when children are changed (keeping aria-current up-to-date) - const menuItems = responsiveProps.menuItems.map(menuItem => { - return validChildren.find(child => child.key === menuItem.key) ?? menuItem - }) - // This is the case where the viewport is too narrow to show any list item with the more menu. In this case, we only show the dropdown - const onlyMenuVisible = responsiveProps.items.length === 0 - if (__DEV__) { + const validChildren = getValidChildren(children) + // Practically, this is not a conditional hook, it is just making sure this hook runs only on DEV not PROD. // eslint-disable-next-line react-hooks/rules-of-hooks useEffect(() => { @@ -203,245 +54,50 @@ export const UnderlineNav = forwardRef( }) } - function getItemsWidth(itemText: string): number { - return noIconChildWidthArray.find(item => item.text === itemText)?.width ?? 0 - } - - const swapMenuItemWithListItem = ( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - prospectiveListItem: React.ReactElement, - indexOfProspectiveListItem: number, - event: React.MouseEvent | React.KeyboardEvent, - callback: (props: ResponsiveProps, displayIcons: boolean, overflowMeasured: boolean) => void, - ) => { - // get the selected menu item's width - const widthToFitIntoList = getItemsWidth(prospectiveListItem.props.children) - // Check if there is any empty space on the right side of the list - const availableSpace = - (navRef.current?.getBoundingClientRect().width ?? 0) - (listRef.current?.getBoundingClientRect().width ?? 0) - - // Calculate how many items need to be pulled in to the menu to make room for the selected menu item - // I.e. if we need to pull 2 items in (index 0 and index 1), breakpoint (index) will return 1. - const index = getBreakpointForItemSwapping(widthToFitIntoList, availableSpace) - const indexToSliceAt = responsiveProps.items.length - 1 - index - // Form the new list of items - const itemsLeftInList = [...responsiveProps.items].slice(0, indexToSliceAt) - const updatedItemList = [...itemsLeftInList, prospectiveListItem] - // Form the new menu items - const itemsToAddToMenu = [...responsiveProps.items].slice(indexToSliceAt) - const updatedMenuItems = [...menuItems] - // Add itemsToAddToMenu array's items to the menu at the index of the prospectiveListItem and remove 1 count of items (prospectiveListItem) - updatedMenuItems.splice(indexOfProspectiveListItem, 1, ...itemsToAddToMenu) - callback({items: updatedItemList, menuItems: updatedMenuItems}, false, true) - } - // How many items do we need to pull in to the menu to make room for the selected menu item. - function getBreakpointForItemSwapping(widthToFitIntoList: number, availableSpace: number) { - let widthToSwap = 0 - let breakpoint = 0 - for (const [index, item] of [...responsiveProps.items].reverse().entries()) { - widthToSwap += getItemsWidth(item.props.children) - if (widthToFitIntoList < widthToSwap + availableSpace) { - breakpoint = index - break - } - } - return breakpoint - } - - const updateListAndMenu = useCallback( - (props: ResponsiveProps, displayIcons: boolean, overflowMeasured: boolean) => { - setResponsiveProps(props) - setIconsVisible(displayIcons) - - if (overflowMeasured) { - setIsOverflowMeasured(true) - } - }, - [], - ) - const setChildrenWidth = useCallback((size: ChildSize) => { - setChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) - - const setNoIconChildrenWidth = useCallback((size: ChildSize) => { - setNoIconChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) - - const closeOverlay = React.useCallback(() => { - setIsWidgetOpen(false) - }, [setIsWidgetOpen]) - - const focusOnMoreMenuBtn = React.useCallback(() => { - moreMenuBtnRef.current?.focus() - }, []) - - const onAnchorClick = useCallback((event: React.MouseEvent) => { - if (event.defaultPrevented || event.button !== 0) { - return - } - setIsWidgetOpen(isWidgetOpen => !isWidgetOpen) - }, []) + const [hideIcons, setHideIcons] = useState(false) - useOnEscapePress( - (event: KeyboardEvent) => { - if (isWidgetOpen) { - event.preventDefault() - closeOverlay() - focusOnMoreMenuBtn() - } - }, - [isWidgetOpen], - ) - - useOnOutsideClick({onClickOutside: closeOverlay, containerRef, ignoreClickRefs: [moreMenuBtnRef]}) + const scrollContainer = useRef(null) + useEffect(() => { + if (hideIcons || !scrollContainer.current) return - useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { - const navWidth = resizeObserverEntries[0].contentRect.width - const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 - navWidth !== 0 && - overflowEffect( - navWidth, - moreMenuWidth, - validChildren, - childWidthArray, - noIconChildWidthArray, - updateListAndMenu, - ) - }, navRef as RefObject) + // Putting the resizeobserver on the list ensures it runs if the contents change; + // the container is fullwidth but the list is elastic + const list = scrollContainer.current.querySelector('ul') + if (!list) return - // Compute menuInlineStyles if needed - let menuInlineStyles: React.CSSProperties = {...baseMenuInlineStyles} - if (containerRef.current && listRef.current) { - const {left} = getAnchoredPosition(containerRef.current, listRef.current, { - align: 'start', - side: 'outside-bottom', + const observer = new ResizeObserver(() => { + if (scrollContainer.current!.scrollWidth > scrollContainer.current!.clientWidth) setHideIcons(true) }) + observer.observe(list) - menuInlineStyles = { - ...baseMenuInlineStyles, - right: undefined, - left, - } - } + return () => observer.disconnect() + }, [hideIcons]) return ( {ariaLabel && {`${ariaLabel} navigation`}} - - - {listItems} - {menuItems.length > 0 && ( -
  • - {!onlyMenuVisible &&
    } - - = baseMenuMinWidth - ? baseMenuInlineStyles - : menuInlineStyles), - display: isWidgetOpen ? 'block' : 'none', - }} - > - {menuItems.map((menuItem, index) => { - const { - children: menuItemChildren, - counter, - 'aria-current': ariaCurrent, - onSelect, - ...menuItemProps - } = menuItem.props - - // This logic is used to pop the selected item out of the menu and into the list when the navigation is control externally - if (Boolean(ariaCurrent) && ariaCurrent !== 'false') { - const event = new MouseEvent('click') - !onlyMenuVisible && - swapMenuItemWithListItem( - menuItem, - index, - // @ts-ignore - not a big deal because it is internally creating an event but ask help - event as React.MouseEvent, - updateListAndMenu, - ) - } - - return ( - | React.KeyboardEvent, - ) => { - // When there are no items in the list, do not run the swap function as we want to keep everything in the menu. - !onlyMenuVisible && swapMenuItemWithListItem(menuItem, index, event, updateListAndMenu) - closeOverlay() - focusOnMoreMenuBtn() - // fire onSelect event that comes from the UnderlineNav.Item (if it is defined) - typeof onSelect === 'function' && onSelect(event) - }} - {...menuItemProps} - > - - {menuItemChildren} - {loadingCounters ? ( - - ) : ( - counter !== undefined && ( - - {counter} - - ) - )} - - - ) - })} - -
    • - )} -     -     + + {children} + +     ) }, diff --git a/packages/react/src/UnderlineNav/UnderlineNavContext.tsx b/packages/react/src/UnderlineNav/UnderlineNavContext.tsx index f276771efac..610d18e8b38 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavContext.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavContext.tsx @@ -1,14 +1,7 @@ -import type React from 'react' import {createContext} from 'react' export const UnderlineNavContext = createContext<{ - setChildrenWidth: React.Dispatch<{text: string; width: number}> - setNoIconChildrenWidth: React.Dispatch<{text: string; width: number}> loadingCounters: boolean - iconsVisible: boolean }>({ - setChildrenWidth: () => null, - setNoIconChildrenWidth: () => null, loadingCounters: false, - iconsVisible: true, }) diff --git a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx index 3341f677e11..664dfe9a1c3 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx @@ -1,11 +1,10 @@ -import type {MutableRefObject, RefObject} from 'react' import React, {forwardRef, useRef, useContext} from 'react' import type {IconProps} from '@primer/octicons-react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {UnderlineNavContext} from './UnderlineNavContext' -import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' import {UnderlineItem} from '../internal/components/UnderlineTabbedInterface' import classes from './UnderlineNavItem.module.css' +import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' // adopted from React.AnchorHTMLAttributes export type LinkProps = { @@ -59,86 +58,88 @@ export type UnderlineNavItemProps = { counter?: number | string } & LinkProps -export const UnderlineNavItem = forwardRef( - ( - { - as: Component = 'a', - href = '#', - children, - counter, - onSelect, - 'aria-current': ariaCurrent, - icon: Icon, - leadingVisual, - ...props +function scrollIntoViewHorizontally(container: HTMLElement, descendant: HTMLElement): void { + // Walk up the offset parent chain to get the true left offset relative to `container` + let offsetLeft = 0 + let el: HTMLElement | null = descendant + while (el && el !== container) { + offsetLeft += el.offsetLeft + el = el.offsetParent as HTMLElement | null + } + + // scrollIntoView would be more convenient but would scroll the entire page unless we pass `options.container`, + // for which browser support is very limited + const descendantLeft = offsetLeft - container.scrollLeft + const descendantRight = descendantLeft + descendant.offsetWidth + + const containerWidth = container.clientWidth + + if (descendantLeft < 0) container.scrollLeft += descendantLeft + else if (descendantRight > containerWidth) container.scrollLeft += descendantRight - containerWidth +} + +export const UnderlineNavItem = forwardRef((allProps, forwardedRef) => { + const { + as: Component = 'a', + href = '#', + children, + counter, + onSelect, + 'aria-current': ariaCurrent, + icon: Icon, + leadingVisual, + ...props + } = allProps + + const ref = useRef(null) + + const {loadingCounters} = useContext(UnderlineNavContext) + + const keyDownHandler = React.useCallback( + (event: React.KeyboardEvent) => { + if ((event.key === ' ' || event.key === 'Enter') && !event.defaultPrevented && typeof onSelect === 'function') { + onSelect(event) + } }, - forwardedRef, - ) => { - const backupRef = useRef(null) - const ref = (forwardedRef ?? backupRef) as RefObject - const {setChildrenWidth, setNoIconChildrenWidth, loadingCounters, iconsVisible} = useContext(UnderlineNavContext) - - useLayoutEffect(() => { - if (ref.current) { - const domRect = (ref as MutableRefObject).current.getBoundingClientRect() - - const icon = Array.from((ref as MutableRefObject).current.children).find( - child => child.getAttribute('data-component') === 'icon', - ) - - const content = Array.from((ref as MutableRefObject).current.children).find( - child => child.getAttribute('data-component') === 'text', - ) as HTMLElement - const text = content.textContent as string - - const iconWidthWithMargin = icon - ? icon.getBoundingClientRect().width + - Number(getComputedStyle(icon).marginRight.slice(0, -2)) + - Number(getComputedStyle(icon).marginLeft.slice(0, -2)) - : 0 - - setChildrenWidth({text, width: domRect.width}) - setNoIconChildrenWidth({text, width: domRect.width - iconWidthWithMargin}) + [onSelect], + ) + + const clickHandler = React.useCallback( + (event: React.MouseEvent) => { + if (!event.defaultPrevented && typeof onSelect === 'function') { + onSelect(event) } - }, [ref, setChildrenWidth, setNoIconChildrenWidth]) - - const keyDownHandler = React.useCallback( - (event: React.KeyboardEvent) => { - if ((event.key === ' ' || event.key === 'Enter') && !event.defaultPrevented && typeof onSelect === 'function') { - onSelect(event) - } - }, - [onSelect], - ) - const clickHandler = React.useCallback( - (event: React.MouseEvent) => { - if (!event.defaultPrevented && typeof onSelect === 'function') { - onSelect(event) - } - }, - [onSelect], - ) - - return ( -
  • - - {children} - -
    • - ) - }, -) as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps> + }, + [onSelect], + ) + + useIsomorphicLayoutEffect(() => { + if (ariaCurrent && ariaCurrent !== 'false' && ref.current) { + const scrollContainer = ref.current.closest('[data-component="underlinenav-scrollcontainer"]') + if (!(scrollContainer instanceof HTMLElement)) return + + scrollIntoViewHorizontally(scrollContainer, ref.current) + } + }, [ariaCurrent]) + + return ( +
  • + + {children} + +
    • + ) +}) as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps> UnderlineNavItem.displayName = 'UnderlineNavItem' diff --git a/packages/react/src/UnderlineNav/styles.ts b/packages/react/src/UnderlineNav/styles.ts deleted file mode 100644 index 71ad7910619..00000000000 --- a/packages/react/src/UnderlineNav/styles.ts +++ /dev/null @@ -1,19 +0,0 @@ -export const dividerStyles = { - display: 'inline-block', - borderLeft: '1px solid', - width: '1px', - borderLeftColor: 'var(--borderColor-muted)', - marginRight: 'var(--base-size-4)', - height: '24px', // The height of the divider - reference from Figma -} - -export const menuItemStyles = { - // This is needed to hide the selected check icon on the menu item. https://github.com/primer/react/tree/main/packages/react/src/ActionList/Selection.tsx#L32 - '& > span': { - display: 'none', - }, - // To reset the style when the menu items are rendered as react router links - textDecoration: 'none', -} - -export const baseMenuMinWidth = 192 diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.module.css b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.module.css index 9e4905c2c7f..ce3d49e5f4f 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.module.css +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.module.css @@ -4,7 +4,3 @@ overflow-y: hidden; -webkit-overflow-scrolling: auto; } - -.StyledUnderlineWrapper[data-icons-visible='false'] [data-component='icon'] { - display: none; -} diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx index 575593ece7d..dd43f61da9c 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx @@ -178,7 +178,7 @@ const UnderlinePanels: FCWithSlotMarker = ({ diff --git a/packages/react/src/internal/components/UnderlineTabbedInterface.module.css b/packages/react/src/internal/components/UnderlineTabbedInterface.module.css index 83cdd50d81f..ce8b2d4aeed 100644 --- a/packages/react/src/internal/components/UnderlineTabbedInterface.module.css +++ b/packages/react/src/internal/components/UnderlineTabbedInterface.module.css @@ -1,32 +1,27 @@ .UnderlineWrapper { display: flex; + padding-top: var(--base-size-8); /* stylelint-disable-next-line primer/spacing */ padding-inline: var(--stack-padding-normal); justify-content: flex-start; - align-items: center; + align-items: flex-start; + width: 100%; /* make space for the underline */ - min-height: var(--control-xlarge-size, 48px); + min-height: var(--control-xlarge-size); /* using a box-shadow instead of a border to accommodate 'overflow-y: hidden' on UnderlinePanels */ /* stylelint-disable-next-line primer/box-shadow */ box-shadow: inset 0 -1px var(--borderColor-muted); - /* Hide overflow until calculation is complete to prevent CLS */ - overflow: visible; - - &[data-overflow-measured='false'] { - overflow: hidden; - } - - &[data-overflow-measured='true'] { - overflow: visible; - } - &[data-variant='flush'] { /* stylelint-disable-next-line primer/spacing */ padding-inline: unset; } + + &[data-hide-icons='true'] [data-component='icon'] { + display: none; + } } .UnderlineItemList { @@ -37,7 +32,7 @@ white-space: nowrap; list-style: none; align-items: center; - gap: 8px; + gap: var(--stack-gap-condensed); } .UnderlineItem { @@ -53,7 +48,10 @@ cursor: pointer; background-color: transparent; border: 0; - border-radius: var(--borderRadius-medium, var(--borderRadius-small)); + border-radius: var(--borderRadius-medium); + max-width: 100%; + margin-bottom: var(--base-size-8); + height: 32px; /* button resets */ appearance: none; @@ -112,20 +110,12 @@ .UnderlineItem::after { position: absolute; - right: 50%; - - /* TODO: see if we can simplify this positioning */ - - /* 48px total height / 2 (24px) + 1px */ - /* stylelint-disable-next-line primer/spacing */ - bottom: calc(50% - calc(var(--control-xlarge-size, var(--base-size-48)) / 2 + 1px)); - width: 100%; + inset: auto 0 0; + margin-bottom: calc(-1 * var(--base-size-8)); height: 2px; pointer-events: none; content: ''; background-color: transparent; - border-radius: 0; - transform: translate(50%, -50%); } .UnderlineItem[aria-current]:not([aria-current='false']) [data-component='text'], diff --git a/packages/react/src/internal/components/UnderlineTabbedInterface.tsx b/packages/react/src/internal/components/UnderlineTabbedInterface.tsx index 509459ea47f..3e7de268225 100644 --- a/packages/react/src/internal/components/UnderlineTabbedInterface.tsx +++ b/packages/react/src/internal/components/UnderlineTabbedInterface.tsx @@ -1,7 +1,7 @@ // Used for UnderlineNav and UnderlinePanels components import React from 'react' -import {type ForwardedRef, forwardRef, type FC, type PropsWithChildren, type ElementType} from 'react' +import {type ForwardedRef, forwardRef, type FC, type ElementType} from 'react' import {isElement} from 'react-is' import type {IconProps} from '@primer/octicons-react' import CounterLabel from '../../CounterLabel' @@ -36,6 +36,7 @@ type UnderlineWrapperProps = { export const UnderlineWrapper = forwardRef((props, ref) => { const {children, className, as: Component = 'div', ...rest} = props + return ( { ) }) as PolymorphicForwardRefComponent> -export const UnderlineItemList = forwardRef(({children, ...rest}: PropsWithChildren, forwardedRef) => { +export const UnderlineItemList = forwardRef(({children, className, ...rest}, forwardedRef) => { return ( -
      +
        {children}
      ) @@ -62,7 +63,6 @@ export const LoadingCounter = () => { export type UnderlineItemProps = { as?: As | 'a' | 'button' className?: string - iconsVisible?: boolean loadingCounters?: boolean counter?: number | string // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -72,11 +72,11 @@ export type UnderlineItemProps = { } & React.ComponentPropsWithoutRef export const UnderlineItem = React.forwardRef((props, ref) => { - const {as: Component = 'a', children, counter, icon: Icon, iconsVisible, loadingCounters, className, ...rest} = props + const {as: Component = 'a', children, counter, icon: Icon, loadingCounters, className, ...rest} = props const textContent = getTextContent(children) return ( - {iconsVisible && Icon && {isElement(Icon) ? Icon : }} + {Icon && {isElement(Icon) ? Icon : }} {children && ( {children}