From f75cb4d811d700695974e67af656bda93ec89151 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Wed, 11 Mar 2026 18:22:23 +0000 Subject: [PATCH 01/12] Replace `ActionBar` overflow calculations with CSS wrapping approach --- .../react/src/ActionBar/ActionBar.module.css | 21 ++ packages/react/src/ActionBar/ActionBar.tsx | 233 ++++++------------ 2 files changed, 90 insertions(+), 164 deletions(-) diff --git a/packages/react/src/ActionBar/ActionBar.module.css b/packages/react/src/ActionBar/ActionBar.module.css index 13aab764f53..fe805f1f7b9 100644 --- a/packages/react/src/ActionBar/ActionBar.module.css +++ b/packages/react/src/ActionBar/ActionBar.module.css @@ -44,6 +44,8 @@ content: ''; /* stylelint-disable-next-line primer/colors */ background: var(--borderColor-muted); + /* stylelint-disable-next-line primer/spacing */ + margin-top: calc((var(--actionbar-height) - var(--base-size-20)) / 2); } } @@ -51,3 +53,22 @@ display: flex; gap: inherit; } + +.OverflowContainer { + display: flex; + flex-wrap: wrap; + gap: inherit; + overflow: hidden; + height: var(--actionbar-height); + justify-content: flex-end; + + --actionbar-height: var(--control-small-size); + + &[data-size='medium'] { + --actionbar-height: var(--control-medium-size); + } + + &[data-size='large'] { + --actionbar-height: var(--control-large-size); + } +} diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx index 83712ce21fd..fa4b003a545 100644 --- a/packages/react/src/ActionBar/ActionBar.tsx +++ b/packages/react/src/ActionBar/ActionBar.tsx @@ -1,10 +1,7 @@ import type {RefObject, MouseEventHandler} from 'react' -import React, {useState, useCallback, useRef, forwardRef, useMemo} from 'react' +import React, {useState, useCallback, useRef, forwardRef, useMemo, useSyncExternalStore} from 'react' import {KebabHorizontalIcon} from '@primer/octicons-react' import {ActionList, type ActionListItemProps} from '../ActionList' -import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' -import type {ResizeObserverEntry} from '../hooks/useResizeObserver' -import {useResizeObserver} from '../hooks/useResizeObserver' import type {IconButtonProps} from '../Button' import {IconButton} from '../Button' @@ -15,8 +12,6 @@ import {clsx} from 'clsx' import {useRefObjectAsForwardedRef} from '../hooks' import {createDescendantRegistry} from '../utils/descendant-registry' -const ACTIONBAR_ITEM_GAP = 8 - type ChildProps = | { type: 'action' @@ -24,32 +19,22 @@ type ChildProps = disabled: boolean icon: ActionBarIconButtonProps['icon'] onClick: MouseEventHandler - width: number groupId?: string } - | {type: 'divider' | 'group'; width: number} + | {type: 'divider' | 'group'} | { type: 'menu' - width: number label: string icon: ActionBarIconButtonProps['icon'] | 'none' items: ActionBarMenuProps['items'] returnFocusRef?: React.RefObject } -/** - * Registry of descendants to render in the list or menu. To preserve insertion order across updates, children are - * set to `null` when unregistered rather than fully dropped from the map. - */ -type ChildRegistry = ReadonlyMap - const ActionBarContext = React.createContext<{ size: Size - isVisibleChild: (id: string) => boolean groupId?: string }>({ size: 'medium', - isVisibleChild: () => true, groupId: undefined, }) @@ -157,30 +142,7 @@ export type ActionBarMenuProps = { returnFocusRef?: React.RefObject } & IconButtonProps -const MORE_BTN_WIDTH = 32 - -const ActionBarItemsRegistry = createDescendantRegistry() - -const calculatePossibleItems = ( - registryEntries: Array<[string, ChildProps]>, - navWidth: number, - gap: number, - moreMenuWidth = 0, -) => { - const widthToFit = navWidth - moreMenuWidth - let breakpoint = registryEntries.length // assume all items will fit - let sumsOfChildWidth = 0 - for (const [index, [, child]] of registryEntries.entries()) { - sumsOfChildWidth += index > 0 ? child.width + gap : child.width - if (sumsOfChildWidth > widthToFit) { - breakpoint = index - break - } else { - continue - } - } - return breakpoint -} +const ActionBarItemsRegistry = createDescendantRegistry() const renderMenuItem = (item: ActionBarMenuItemProps, index: number): React.ReactNode => { if (item.type === 'divider') { @@ -231,60 +193,6 @@ const renderMenuItem = (item: ActionBarMenuItemProps, index: number): React.Reac ) } -const getMenuItems = ( - navWidth: number, - moreMenuWidth: number, - childRegistry: ChildRegistry | undefined, - hasActiveMenu: boolean, - gap: number, -): Set | void => { - const registryEntries = Array.from(childRegistry?.entries() ?? []).filter( - (entry): entry is [string, ChildProps] => - entry[1] !== null && (entry[1].type !== 'action' || entry[1].groupId === undefined), - ) - - if (registryEntries.length === 0) return new Set() - const numberOfItemsPossible = calculatePossibleItems(registryEntries, navWidth, gap) - - const numberOfItemsPossibleWithMoreMenu = calculatePossibleItems( - registryEntries, - navWidth, - gap, - moreMenuWidth || MORE_BTN_WIDTH, - ) - const menuItems = new Set() - - // First, we check if we can fit all the items with their icons - if (registryEntries.length >= numberOfItemsPossible) { - /* 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 = registryEntries.length - numberOfItemsPossibleWithMoreMenu - const numberOfListItems = - numberOfItemsInMenu === 1 ? numberOfItemsPossibleWithMoreMenu - 1 : numberOfItemsPossibleWithMoreMenu - for (const [index, [id, child]] of registryEntries.entries()) { - if (index < numberOfListItems) { - continue - //if the last item is a divider - } else if (child.type === 'divider') { - if (index === numberOfListItems - 1 || index === numberOfListItems) { - continue - } else { - menuItems.add(id) - } - } else { - menuItems.add(id) - } - } - - return menuItems - } else if (numberOfItemsPossible > registryEntries.length && hasActiveMenu) { - /* If the items fit in the list and there are items in the overflow menu, we need to move them back to the list */ - return new Set() - } -} - export const ActionBar: React.FC> = props => { const { size = 'medium', @@ -298,39 +206,10 @@ export const ActionBar: React.FC> = prop // We derive the numeric gap from computed style so layout math stays in sync with CSS const listRef = useRef(null) - const [computedGap, setComputedGap] = useState(ACTIONBAR_ITEM_GAP) const [childRegistry, setChildRegistry] = ActionBarItemsRegistry.useRegistryState() - const [menuItemIds, setMenuItemIds] = useState>(() => new Set()) - const navRef = useRef(null) - // measure gap after first render & whenever gap scale changes - useIsomorphicLayoutEffect(() => { - if (!listRef.current) return - const g = window.getComputedStyle(listRef.current).gap - const parsed = parseFloat(g) - if (!Number.isNaN(parsed)) setComputedGap(parsed) - }, [gap]) - const moreMenuRef = useRef(null) - - useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { - const navWidth = resizeObserverEntries[0].contentRect.width - const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 - const hasActiveMenu = menuItemIds.size > 0 - - if (navWidth > 0) { - const newMenuItemIds = getMenuItems(navWidth, moreMenuWidth, childRegistry, hasActiveMenu, computedGap) - if (newMenuItemIds) setMenuItemIds(newMenuItemIds) - } - }, navRef as RefObject) - - const isVisibleChild = useCallback( - (id: string) => { - return !menuItemIds.has(id) - }, - [menuItemIds], - ) useFocusZone({ containerRef: listRef, @@ -338,11 +217,15 @@ export const ActionBar: React.FC> = prop focusOutBehavior: 'wrap', }) + const overflowItems = Array.from(childRegistry?.entries() ?? []).filter( + (entry): entry is [string, ChildProps] => entry[1] !== null, + ) + const groupedItems = React.useMemo(() => { const groupedItemsMap = new Map>() for (const [key, childProps] of childRegistry ?? []) { - if (childProps.type === 'action' && childProps.groupId) { + if (childProps?.type === 'action' && childProps.groupId) { const existingGroup = groupedItemsMap.get(childProps.groupId) || [] existingGroup.push([key, childProps]) groupedItemsMap.set(childProps.groupId, existingGroup) @@ -352,7 +235,7 @@ export const ActionBar: React.FC> = prop }, [childRegistry]) return ( - +
> = prop aria-labelledby={ariaLabelledBy} data-gap={gap} > - {children} - {menuItemIds.size > 0 && ( +
+ {children} +
+ {overflowItems.length > 0 && ( - {Array.from(menuItemIds).map(id => { - const menuItem = childRegistry?.get(id) - if (!menuItem) return null - + {overflowItems.map(([id, menuItem]) => { if (menuItem.type === 'divider') { return - } else if (menuItem.type === 'action') { + } + + if (menuItem.type === 'action') { const {onClick, icon: Icon, label, disabled} = menuItem return ( > = prop ) } -function useWidth(ref: React.RefObject) { - const [width, setWidth] = useState(-1) - - useIsomorphicLayoutEffect(() => setWidth(ref.current?.getBoundingClientRect().width ?? -1), [ref]) - - return width +function useIsOverflowing(ref: React.RefObject) { + return useSyncExternalStore( + useCallback( + onChange => { + const observer = new IntersectionObserver(() => onChange(), { + threshold: 1, + }) + if (ref.current) observer.observe(ref.current) + return () => observer.disconnect() + }, + [ref], + ), + // Note: the IntersectionObserver is just being used as a trigger to re-check + // `offsetTop > 0`; this is fast and simpler than checking visibility from + // the observed entry. When an item wraps, it will move to the next row which + // increases its `offsetTop` + () => (ref.current ? ref.current.offsetTop > 0 : false), + () => false, + ) } export const ActionBarIconButton = forwardRef( @@ -472,10 +369,10 @@ export const ActionBarIconButton = forwardRef( const ref = useRef(null) useRefObjectAsForwardedRef(forwardedRef, ref) - const {size, isVisibleChild} = React.useContext(ActionBarContext) + const {size} = React.useContext(ActionBarContext) const {groupId} = React.useContext(ActionBarGroupContext) - const width = useWidth(ref) + const isOverflowing = useIsOverflowing(ref) const {['aria-label']: ariaLabel, icon} = props @@ -486,13 +383,12 @@ export const ActionBarIconButton = forwardRef( icon, disabled: !!disabled, onClick: onClick as MouseEventHandler, - width, groupId: groupId ?? undefined, }), - [ariaLabel, icon, disabled, onClick, groupId, width], + [ariaLabel, icon, disabled, onClick, groupId], ) - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + ActionBarItemsRegistry.useRegisterDescendant(isOverflowing ? registryProps : null) const clickHandler = useCallback( (event: React.MouseEvent) => { @@ -502,8 +398,6 @@ export const ActionBarIconButton = forwardRef( [disabled, onClick], ) - if (!isVisibleChild(id) || (groupId && !isVisibleChild(groupId))) return null - return ( ) }, @@ -524,15 +420,15 @@ const ActionBarGroupContext = React.createContext<{ export const ActionBarGroup = forwardRef(({children}: React.PropsWithChildren, forwardedRef) => { const backupRef = useRef(null) const ref = (forwardedRef ?? backupRef) as RefObject - const width = useWidth(ref) + const isOverflowing = useIsOverflowing(ref) - const registryProps = useMemo((): ChildProps => ({type: 'group', width}), [width]) + const registryProps = useMemo((): ChildProps => ({type: 'group'}), []) - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + const id = ActionBarItemsRegistry.useRegisterDescendant(isOverflowing ? registryProps : null) return ( -
+
{children}
@@ -546,32 +442,35 @@ export const ActionBarMenu = forwardRef( ) => { const backupRef = useRef(null) const ref = (forwardedRef ?? backupRef) as RefObject - const {isVisibleChild} = React.useContext(ActionBarContext) const [menuOpen, setMenuOpen] = useState(false) - const width = useWidth(ref) + const isOverflowing = useIsOverflowing(ref) const registryProps = useMemo( (): ChildProps => ({ type: 'menu', - width, label: ariaLabel, icon: overflowIcon ? overflowIcon : icon, returnFocusRef, items, }), - [ariaLabel, overflowIcon, icon, items, returnFocusRef, width], + [ariaLabel, overflowIcon, icon, items, returnFocusRef], ) - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) - - if (!isVisibleChild(id)) return null + ActionBarItemsRegistry.useRegisterDescendant(isOverflowing ? registryProps : null) return ( - + {items.map((item, index) => renderMenuItem(item, index))} @@ -583,14 +482,20 @@ export const ActionBarMenu = forwardRef( export const VerticalDivider = () => { const ref = useRef(null) - const {isVisibleChild} = React.useContext(ActionBarContext) - const width = useWidth(ref) + const isOverflowing = useIsOverflowing(ref) - const registryProps = useMemo((): ChildProps => ({type: 'divider', width}), [width]) + const registryProps = useMemo((): ChildProps => ({type: 'divider'}), []) - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + ActionBarItemsRegistry.useRegisterDescendant(isOverflowing ? registryProps : null) - if (!isVisibleChild(id)) return null - return