diff --git a/.changeset/derive-autocomplete-labelgroup-state-in-render.md b/.changeset/derive-autocomplete-labelgroup-state-in-render.md new file mode 100644 index 00000000000..67cfa116ac1 --- /dev/null +++ b/.changeset/derive-autocomplete-labelgroup-state-in-render.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Derive `AutocompleteMenu` close-time re-sorting and `LabelGroup` numeric truncation during render instead of in effects, removing redundant render passes. No public API or visual changes. diff --git a/packages/react/src/Autocomplete/Autocomplete.test.tsx b/packages/react/src/Autocomplete/Autocomplete.test.tsx index 054a9b52e0a..7caf9d195a8 100644 --- a/packages/react/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/react/src/Autocomplete/Autocomplete.test.tsx @@ -289,50 +289,61 @@ describe('Autocomplete', () => { }) it('calls a custom sort function when the menu closes', async () => { - const user = userEvent.setup() const sortOnCloseFnMock = vi.fn() + const onOpenChangeMock = vi.fn() const {container} = render( , ) - const inputNode = container.querySelector('#autocompleteInput') + const inputNode = container.querySelector('#autocompleteInput') as HTMLInputElement - // `sortOnCloseFnMock` will be called in a `.sort()` on render to check if the - // current sort order matches the result of `sortOnCloseFnMock` - expect(sortOnCloseFnMock).toHaveBeenCalledTimes(mockItems.length - 1) - if (inputNode) { - await user.type(inputNode, 'ze') - // eslint-disable-next-line github/no-blur - fireEvent.blur(inputNode) - } + // The menu starts closed, so the items are sorted during render. + expect(sortOnCloseFnMock).toHaveBeenCalled() - // wait a tick for blur to finish + // Opening the menu does not run the close-time sort. + sortOnCloseFnMock.mockClear() + fireEvent.click(inputNode) + fireEvent.keyDown(inputNode, {key: 'ArrowDown'}) + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(true)) + expect(sortOnCloseFnMock).not.toHaveBeenCalled() + + // Closing the menu re-runs the sort. + // eslint-disable-next-line github/no-blur + fireEvent.blur(inputNode) await waitFor(() => expect(sortOnCloseFnMock).toHaveBeenCalled()) }) it("calls onOpenChange with the menu's open state", async () => { - const user = userEvent.setup() const onOpenChangeMock = vi.fn() - const {container} = render( + const {getByLabelText} = render( , ) - const inputNode = container.querySelector('#autocompleteInput') + const inputNode = getByLabelText(AUTOCOMPLETE_LABEL) - inputNode && (await user.type(inputNode, 'ze')) - expect(onOpenChangeMock).toHaveBeenCalled() + expect(onOpenChangeMock).toHaveBeenNthCalledWith(1, false) + + fireEvent.click(inputNode) + fireEvent.keyDown(inputNode, {key: 'ArrowDown'}) + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(true)) + + // eslint-disable-next-line github/no-blur + fireEvent.blur(inputNode) + + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(false)) }) it('calls onSelectedChange with the data for the selected items', async () => { diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index b87472c3149..672be5835cc 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -324,7 +324,13 @@ function AutocompleteMenu(props: AutocompleteMe } }, [highlightedItem, deferredInputValue, selectedItemIds, setAutocompleteSuggestion]) - useEffect(() => { + // Re-sort the items during render while the menu is closed so the order is ready the next time + // it opens. `AutocompleteOverlay` mounts this component under a different parent when the menu + // opens vs. closes, so it remounts on each open/close and a render-time previous-value transition + // can't be observed here — guarding on the closed state is what replaces the old close-time effect. + // The comparator therefore runs only while the menu is closed, never on the frequent renders that + // happen while it is open. + if (showMenu === false) { const itemIdSortResult = [...sortedItemIds].sort( sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), ) @@ -332,18 +338,15 @@ function AutocompleteMenu(props: AutocompleteMe itemIdSortResult.length === sortedItemIds.length && itemIdSortResult.every((element, index) => element === sortedItemIds[index]) - // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler - if (showMenu === false && !sortResultMatchesState) { - // Re-sort only when the menu closes. This effect also fires `onOpenChange` below, so it - // stays an effect. (Follow-up: the sort could be derived on the open→closed transition - // while keeping `onOpenChange` in an effect.) - // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state + if (!sortResultMatchesState) { setSortedItemIds(itemIdSortResult) } + } + useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent onOpenChange && onOpenChange(Boolean(showMenu)) - }, [showMenu, onOpenChange, selectedItemIds, sortOnCloseFn, sortedItemIds]) + }, [showMenu, onOpenChange]) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler diff --git a/packages/react/src/LabelGroup/LabelGroup.test.tsx b/packages/react/src/LabelGroup/LabelGroup.test.tsx index d35c143d599..f2f40af83dd 100644 --- a/packages/react/src/LabelGroup/LabelGroup.test.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.test.tsx @@ -112,7 +112,7 @@ describe('LabelGroup', () => { }) it('should truncate labels to a specified number', () => { - const {getByText} = render( + const {getByText, queryByText, rerender} = render( @@ -126,6 +126,21 @@ describe('LabelGroup', () => { const expandButton = getByText('+2') expect(expandButton).toBeDefined() + + rerender( + + + + + + + + + , + ) + + expect(queryByText('+2')).toBeNull() + expect(getByText('+1')).toBeDefined() }) it('should expand all tokens into an overlay when overflowStyle="overlay"', async () => { diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index fa91230cb56..b3fe7a5b133 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -23,6 +23,26 @@ const getOverlayWidth = ( overlayPaddingPx: number, ) => overlayPaddingPx + buttonClientRect.right - (containerRef.current?.getBoundingClientRect().left || 0) +const getVisibilityMapAfterIndex = (childCount: number, truncateAfter: number) => { + const updatedEntries: Record = {} + + for (let index = 0; index < childCount; index++) { + updatedEntries[index.toString()] = index < truncateAfter + } + + return updatedEntries +} + +const visibilityMapsMatch = (left: Record, right: Record) => { + const leftKeys = Object.keys(left) + const rightKeys = Object.keys(right) + + return ( + leftKeys.length === rightKeys.length && + leftKeys.every(key => Object.prototype.hasOwnProperty.call(right, key) && left[key] === right[key]) + ) +} + const InlineToggle: React.FC<{ collapseButtonRef: React.RefObject collapseInlineExpandedChildren: () => void @@ -131,6 +151,7 @@ const LabelGroup: React.FC> = ({ bottom: 0, toJSON: () => undefined, }) + const childArray = React.Children.toArray(children) const overlayPaddingPx = 8 // var(--base-size-8), hardcoded to do some math const hiddenItemIds = Object.keys(visibilityMap).filter(key => !visibilityMap[key]) @@ -163,18 +184,12 @@ const LabelGroup: React.FC> = ({ ) // Sets the visibility map to hide children after the given index. - const hideChildrenAfterIndex = React.useCallback((truncateAfter: number) => { - const containerChildren = containerRef.current?.children || [] - const updatedEntries: Record = {} - for (const child of containerChildren) { - const targetId = child.getAttribute('data-index') - if (targetId) { - updatedEntries[targetId] = parseInt(targetId, 10) < truncateAfter - } - } - - setVisibilityMap(updatedEntries) - }, []) + const hideChildrenAfterIndex = React.useCallback( + (truncateAfter: number) => { + setVisibilityMap(getVisibilityMapAfterIndex(childArray.length, truncateAfter)) + }, + [childArray.length], + ) const openOverflowOverlay = React.useCallback(() => setIsOverflowShown(true), [setIsOverflowShown]) @@ -203,6 +218,18 @@ const LabelGroup: React.FC> = ({ setIsOverflowShown(true) }, [setVisibilityMap, setIsOverflowShown]) + const numericVisibilityMap = React.useMemo( + () => + visibleChildCount && typeof visibleChildCount === 'number' && !isOverflowShown + ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) + : undefined, + [childArray.length, isOverflowShown, visibleChildCount], + ) + + if (numericVisibilityMap && !visibilityMapsMatch(visibilityMap, numericVisibilityMap)) { + setVisibilityMap(numericVisibilityMap) + } + React.useEffect(() => { // If we're not truncating, we don't need to run this useEffect. // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler @@ -210,6 +237,7 @@ const LabelGroup: React.FC> = ({ return } + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (visibleChildCount === 'auto') { // Instantiates the IntersectionObserver to track when children fit in the container. const observer = new IntersectionObserver( @@ -245,14 +273,7 @@ const LabelGroup: React.FC> = ({ return () => observer.disconnect() } - // We're not auto truncating, so we need to hide children after the given `visibleChildCount`. - // `hideChildrenAfterIndex` reads the rendered children from the DOM, so it must run in an - // effect. (Follow-up: this branch could derive the visibility map from child indices instead.) - else { - // eslint-disable-next-line react-hooks/set-state-in-effect - hideChildrenAfterIndex(visibleChildCount) - } - }, [buttonClientRect, visibleChildCount, hideChildrenAfterIndex, isOverflowShown]) + }, [buttonClientRect, visibleChildCount, isOverflowShown]) // Updates the index of the first hidden child. // We need to keep track of this so we can focus the first hidden child when the overflow is shown inline. @@ -304,7 +325,7 @@ const LabelGroup: React.FC> = ({ className={clsx(className, classes.Container)} data-component="LabelGroup" > - {React.Children.map(children, (child, index) => ( + {childArray.map((child, index) => ( > = ({ hiddenItemIds={hiddenItemIds} isOverflowShown={isOverflowShown} showAllTokensInline={showAllTokensInline} - totalLength={React.Children.toArray(children).length} + totalLength={childArray.length} /> ) : ( > = ({ openOverflowOverlay={openOverflowOverlay} overlayPaddingPx={overlayPaddingPx} overlayWidth={overlayWidth} - totalLength={React.Children.toArray(children).length} + totalLength={childArray.length} > {children}