diff --git a/.changeset/fair-bars-smile.md b/.changeset/fair-bars-smile.md new file mode 100644 index 00000000000..f8eba55a7e7 --- /dev/null +++ b/.changeset/fair-bars-smile.md @@ -0,0 +1,5 @@ +--- +'@primer/react': major +--- + +Support nested children in ActionBar. diff --git a/packages/react/src/ActionBar/ActionBar.stories.tsx b/packages/react/src/ActionBar/ActionBar.stories.tsx index 30988947ed4..91c24455431 100644 --- a/packages/react/src/ActionBar/ActionBar.stories.tsx +++ b/packages/react/src/ActionBar/ActionBar.stories.tsx @@ -63,3 +63,33 @@ export const Default = () => ( ) + +const BoldButton = () => + +const FormattingButtons = () => ( + <> + + + + + +) + +const AdvancedFormattingButtons = () => ( + <> + + + + + + + +) + +export const DeepChildTree = () => ( + + + + + +) diff --git a/packages/react/src/ActionBar/ActionBar.test.tsx b/packages/react/src/ActionBar/ActionBar.test.tsx index 50555a2eeab..21139a9cb8e 100644 --- a/packages/react/src/ActionBar/ActionBar.test.tsx +++ b/packages/react/src/ActionBar/ActionBar.test.tsx @@ -1,9 +1,9 @@ import {describe, expect, it, afterEach, vi} from 'vitest' import {render, screen, act} from '@testing-library/react' import userEvent from '@testing-library/user-event' - import ActionBar from './' -import {BoldIcon} from '@primer/octicons-react' +import {BoldIcon, ItalicIcon, CodeIcon} from '@primer/octicons-react' +import {useState} from 'react' describe('ActionBar', () => { afterEach(() => { @@ -78,3 +78,159 @@ describe('ActionBar', () => { expect(onClick).toHaveBeenCalled() }) }) + +describe('ActionBar Registry System', () => { + it('should preserve order with deep nesting', () => { + render( + +
+ +
+ +
+ +
+ , + ) + + const buttons = screen.getAllByRole('button') + expect(buttons).toHaveLength(3) + expect(buttons[0]).toHaveAccessibleName('First') + expect(buttons[1]).toHaveAccessibleName('Second') + expect(buttons[2]).toHaveAccessibleName('Third') + }) + + it('should handle conditional rendering without breaking order', async () => { + const ConditionalTest = () => { + const [show, setShow] = useState([true, true, true]) + + return ( +
+ + {show[0] && } + {show[1] && } + {show[2] && } + + + +
+ ) + } + + const user = userEvent.setup() + render() + + // Initially should have 3 buttons + expect(screen.getAllByRole('button', {name: /First|Second|Third/})).toHaveLength(3) + + // Hide first button + await user.click(screen.getByText('Hide first')) + + const buttonsAfterHide = screen.getAllByRole('button', {name: /Second|Third/}) + expect(buttonsAfterHide).toHaveLength(2) + expect(buttonsAfterHide[0]).toHaveAccessibleName('Second') + expect(buttonsAfterHide[1]).toHaveAccessibleName('Third') + + // Show first button again + await user.click(screen.getByText('Show all')) + + const buttonsAfterShow = screen.getAllByRole('button', {name: /First|Second|Third/}) + expect(buttonsAfterShow).toHaveLength(3) + expect(buttonsAfterShow[0]).toHaveAccessibleName('First') + expect(buttonsAfterShow[1]).toHaveAccessibleName('Second') + expect(buttonsAfterShow[2]).toHaveAccessibleName('Third') + }) + + it('should handle fragments and array mapping', () => { + render( + + <> + + {[1, 2].map(i => ( + + ))} + + + , + ) + + const buttons = screen.getAllByRole('button') + expect(buttons).toHaveLength(4) + expect(buttons[0]).toHaveAccessibleName('In fragment') + expect(buttons[1]).toHaveAccessibleName('Mapped 1') + expect(buttons[2]).toHaveAccessibleName('Mapped 2') + expect(buttons[3]).toHaveAccessibleName('After fragment') + }) + + it('should handle rapid re-renders without losing registry data', async () => { + const RapidRerenderTest = () => { + const [count, setCount] = useState(0) + + return ( +
+ + + + +
+ ) + } + + const user = userEvent.setup() + render() + + // Rapidly trigger re-renders + for (let i = 0; i < 10; i++) { + await user.click(screen.getByText('Increment')) + } + + expect(screen.getByRole('button', {name: 'Button 10'})).toBeInTheDocument() + }) + + it('should handle zero-width scenarios gracefully', () => { + render( +
+ + + +
, + ) + + // Component should still render even with zero width + expect(screen.getByRole('button', {name: 'Zero width button'})).toBeInTheDocument() + }) + + it('should clean up registry on unmount', async () => { + const UnmountTest = () => { + const [mounted, setMounted] = useState(true) + + return ( +
+ {mounted && ( + + + + )} + +
+ ) + } + + const user = userEvent.setup() + render() + + expect(screen.getByRole('button', {name: 'Will unmount'})).toBeInTheDocument() + + await user.click(screen.getByText('Unmount')) + + expect(screen.queryByRole('button', {name: 'Will unmount'})).not.toBeInTheDocument() + }) +}) diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx index d4374077cab..f4f718794fa 100644 --- a/packages/react/src/ActionBar/ActionBar.tsx +++ b/packages/react/src/ActionBar/ActionBar.tsx @@ -1,5 +1,5 @@ -import type {RefObject, MutableRefObject} from 'react' -import React, {useState, useCallback, useRef, forwardRef} from 'react' +import type {RefObject, MouseEventHandler} from 'react' +import React, {useState, useCallback, useRef, forwardRef, useId} from 'react' import {KebabHorizontalIcon} from '@primer/octicons-react' import {ActionList} from '../ActionList' import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' @@ -14,23 +14,33 @@ import {ActionMenu} from '../ActionMenu' import {useFocusZone, FocusKeys} from '../hooks/useFocusZone' import styles from './ActionBar.module.css' import {clsx} from 'clsx' +import {useRefObjectAsForwardedRef} from '../hooks' const ACTIONBAR_ITEM_GAP = 8 -type ChildSize = { - text: string - width: number -} -type ChildWidthArray = Array -type ResponsiveProps = { - items: Array - menuItems: Array -} +type ChildProps = + | { + type: 'action' + label: string + disabled: boolean + icon: ActionBarIconButtonProps['icon'] + onClick: MouseEventHandler + width: number + } + | {type: 'divider'; width: number} + +/** + * 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 - setChildrenWidth: React.Dispatch<{text: string; width: number}> -}>({size: 'medium', setChildrenWidth: () => null}) + registerChild: (id: string, props: ChildProps) => void + unregisterChild: (id: string) => void + isVisibleChild: (id: string) => boolean +}>({size: 'medium', registerChild: () => {}, unregisterChild: () => {}, isVisibleChild: () => true}) /* small (28px), medium (32px), large (40px) @@ -75,18 +85,12 @@ export type ActionBarIconButtonProps = {disabled?: boolean} & IconButtonProps const MORE_BTN_WIDTH = 32 -const getValidChildren = (children: React.ReactNode) => { - return React.Children.toArray(children).filter(child => { - return React.isValidElement(child) - }) as React.ReactElement[] -} - -const calculatePossibleItems = (childWidthArray: ChildWidthArray, navWidth: number, moreMenuWidth = 0) => { +const calculatePossibleItems = (registryEntries: Array<[string, ChildProps]>, navWidth: number, moreMenuWidth = 0) => { const widthToFit = navWidth - moreMenuWidth - let breakpoint = childWidthArray.length // assume all items will fit + let breakpoint = registryEntries.length // assume all items will fit let sumsOfChildWidth = 0 - for (const [index, childWidth] of childWidthArray.entries()) { - sumsOfChildWidth += index > 0 ? childWidth.width + ACTIONBAR_ITEM_GAP : childWidth.width + for (const [index, [, child]] of registryEntries.entries()) { + sumsOfChildWidth += index > 0 ? child.width + ACTIONBAR_ITEM_GAP : child.width if (sumsOfChildWidth > widthToFit) { breakpoint = index break @@ -97,68 +101,67 @@ const calculatePossibleItems = (childWidthArray: ChildWidthArray, navWidth: numb return breakpoint } -const overflowEffect = ( +const getMenuItems = ( navWidth: number, moreMenuWidth: number, - childArray: Array, - childWidthArray: ChildWidthArray, - updateListAndMenu: (props: ResponsiveProps) => void, + childRegistry: ChildRegistry, hasActiveMenu: boolean, -) => { - if (childWidthArray.length === 0) { - updateListAndMenu({items: childArray, menuItems: []}) - } - const numberOfItemsPossible = calculatePossibleItems(childWidthArray, navWidth) +): Set | void => { + const registryEntries = Array.from(childRegistry).filter((entry): entry is [string, ChildProps] => entry[1] !== null) + + if (registryEntries.length === 0) return new Set() + const numberOfItemsPossible = calculatePossibleItems(registryEntries, navWidth) const numberOfItemsPossibleWithMoreMenu = calculatePossibleItems( - childWidthArray, + registryEntries, navWidth, moreMenuWidth || MORE_BTN_WIDTH, ) - const items: Array = [] - const menuItems: Array = [] + const menuItems = new Set() // First, we check if we can fit all the items with their icons - if (childArray.length >= numberOfItemsPossible) { + 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 = childArray.length - numberOfItemsPossibleWithMoreMenu + const numberOfItemsInMenu = registryEntries.length - numberOfItemsPossibleWithMoreMenu const numberOfListItems = numberOfItemsInMenu === 1 ? numberOfItemsPossibleWithMoreMenu - 1 : numberOfItemsPossibleWithMoreMenu - for (const [index, child] of childArray.entries()) { + for (const [index, [id, child]] of registryEntries.entries()) { if (index < numberOfListItems) { - items.push(child) + continue //if the last item is a divider - } else if (childWidthArray[index].text === 'divider') { + } else if (child.type === 'divider') { if (index === numberOfListItems - 1 || index === numberOfListItems) { continue } else { - const divider = React.createElement(ActionList.Divider, {key: index}) - menuItems.push(divider) + menuItems.add(id) } } else { - menuItems.push(child) + menuItems.add(id) } } - updateListAndMenu({items, menuItems}) - } else if (numberOfItemsPossible > childArray.length && hasActiveMenu) { + 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 */ - updateListAndMenu({items: childArray, menuItems: []}) + return new Set() } } export const ActionBar: React.FC> = props => { const {size = 'medium', children, 'aria-label': ariaLabel, flush = false, className} = props - const [childWidthArray, setChildWidthArray] = useState([]) - const setChildrenWidth = useCallback((size: ChildSize) => { - setChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) + + const [childRegistry, setChildRegistry] = useState(() => new Map()) + + const registerChild = useCallback( + (id: string, childProps: ChildProps) => setChildRegistry(prev => new Map(prev).set(id, childProps)), + [], + ) + const unregisterChild = useCallback((id: string) => setChildRegistry(prev => new Map(prev).set(id, null)), []) + + const [menuItemIds, setMenuItemIds] = useState>(() => new Set()) const navRef = useRef(null) const listRef = useRef(null) @@ -166,35 +169,24 @@ export const ActionBar: React.FC> = prop const moreMenuBtnRef = useRef(null) const containerRef = React.useRef(null) - 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 - }) - - const updateListAndMenu = useCallback((props: ResponsiveProps) => { - setResponsiveProps(props) - }, []) - useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { const navWidth = resizeObserverEntries[0].contentRect.width const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 - const hasActiveMenu = menuItems.length > 0 - navWidth !== 0 && - overflowEffect(navWidth, moreMenuWidth, validChildren, childWidthArray, updateListAndMenu, hasActiveMenu) + const hasActiveMenu = menuItemIds.size > 0 + + if (navWidth > 0) { + const newMenuItemIds = getMenuItems(navWidth, moreMenuWidth, childRegistry, hasActiveMenu) + if (newMenuItemIds) setMenuItemIds(newMenuItemIds) + } }, navRef as RefObject) + const isVisibleChild = useCallback( + (id: string) => { + return !menuItemIds.has(id) + }, + [menuItemIds], + ) + const [isWidgetOpen, setIsWidgetOpen] = useState(false) const closeOverlay = React.useCallback(() => { @@ -225,31 +217,28 @@ export const ActionBar: React.FC> = prop }) return ( - +
- {listItems} - {menuItems.length > 0 && ( + {children} + {menuItemIds.size > 0 && ( - {menuItems.map((menuItem, index) => { - if (menuItem.type === ActionList.Divider) { - return + {Array.from(menuItemIds).map(id => { + const menuItem = childRegistry.get(id) + if (!menuItem) return null + + if (menuItem.type === 'divider') { + return } else { - const { - children: menuItemChildren, - onClick, - icon: Icon, - 'aria-label': ariaLabel, - disabled, - } = menuItem.props + const {onClick, icon: Icon, label, disabled} = menuItem return ( ) => { closeOverlay() @@ -258,12 +247,10 @@ export const ActionBar: React.FC> = prop }} disabled={disabled} > - {Icon ? ( - - - - ) : null} - {ariaLabel} + + + + {label} ) } @@ -280,14 +267,31 @@ export const ActionBar: React.FC> = prop export const ActionBarIconButton = forwardRef( ({disabled, onClick, ...props}: ActionBarIconButtonProps, forwardedRef) => { - const backupRef = useRef(null) - const ref = (forwardedRef ?? backupRef) as RefObject - const {size, setChildrenWidth} = React.useContext(ActionBarContext) + const ref = useRef(null) + useRefObjectAsForwardedRef(forwardedRef, ref) + const id = useId() + + const {size, registerChild, unregisterChild, isVisibleChild} = React.useContext(ActionBarContext) + + // Storing the width in a ref ensures we don't forget about it when not visible + const widthRef = useRef() + useIsomorphicLayoutEffect(() => { - const text = props['aria-label'] ? props['aria-label'] : '' - const domRect = (ref as MutableRefObject).current.getBoundingClientRect() - setChildrenWidth({text, width: domRect.width}) - }, [ref, setChildrenWidth]) + const width = ref.current?.getBoundingClientRect().width + if (width) widthRef.current = width + if (!widthRef.current) return + + registerChild(id, { + type: 'action', + label: props['aria-label'] ?? '', + icon: props.icon, + disabled: !!disabled, + onClick: onClick as MouseEventHandler, + width: widthRef.current, + }) + + return () => unregisterChild(id) + }, [registerChild, unregisterChild, props['aria-label'], props.icon, disabled, onClick]) const clickHandler = useCallback( (event: React.MouseEvent) => { @@ -297,6 +301,8 @@ export const ActionBarIconButton = forwardRef( [disabled, onClick], ) + if (!isVisibleChild(id)) return null + return ( { const ref = useRef(null) - const {setChildrenWidth} = React.useContext(ActionBarContext) + const id = useId() + const {registerChild, unregisterChild, isVisibleChild} = React.useContext(ActionBarContext) + + // Storing the width in a ref ensures we don't forget about it when not visible + const widthRef = useRef() + useIsomorphicLayoutEffect(() => { - const text = 'divider' - const domRect = (ref as MutableRefObject).current.getBoundingClientRect() - setChildrenWidth({text, width: domRect.width}) - }, [ref, setChildrenWidth]) + const width = ref.current?.getBoundingClientRect().width + if (width) widthRef.current = width + if (!widthRef.current) return + + registerChild(id, {type: 'divider', width: widthRef.current}) + + return () => unregisterChild(id) + }, [registerChild, unregisterChild]) + + if (!isVisibleChild(id)) return null return