From c73102c616edeae3342eb0f44900d455801a7b3e Mon Sep 17 00:00:00 2001 From: Ian Candy Date: Tue, 27 Feb 2024 22:01:53 +0000 Subject: [PATCH 1/3] Allow non-memoized item lists Towards https://github.com/primer/react/issues/4315 The SelectPanel only did a basic equality check for the item state, meaning that it depended on having the exact same objects on multiple pass throughs. This isn't always possible as sometimes you may want to have different objects. This replaces the equality check with a test for an `id` property on the object. If the `id` property isn't present, we fallback to the old behavior. Note that a previous version used the `key` prop, but we decided `id` was a better interface. https://github.com/ipc103/primer-react/commit/f472da25d4f1ff48326c0056aa1696c9c94b4f81#r139163795 Co-authored-by: Andrew Henry --- .../src/SelectPanel/SelectPanel.test.tsx | 36 +++++++++++++++++-- .../react/src/SelectPanel/SelectPanel.tsx | 28 ++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index 3f5b953010b..851b715e2d0 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -1,4 +1,4 @@ -import {render as HTMLRender} from '@testing-library/react' +import {render as HTMLRender, fireEvent, getByRole, getByText} from '@testing-library/react' import {axe} from 'jest-axe' import React from 'react' import theme from '../theme' @@ -7,9 +7,14 @@ import {behavesAsComponent, checkExports} from '../utils/testing' import {BaseStyles, SSRProvider, ThemeProvider} from '..' import type {ItemInput} from '../deprecated/ActionList/List' -const items = [{text: 'Foo'}, {text: 'Bar'}, {text: 'Baz'}, {text: 'Bon'}] as ItemInput[] - function SimpleSelectPanel(): JSX.Element { + const items = [ + {text: 'Foo', id: 'foo'}, + {text: 'Bar', id: 'bar'}, + {text: 'Baz', id: 'baz'}, + {text: 'Bon', id: 'bon'}, + ] as ItemInput[] + const [selected, setSelected] = React.useState([]) const [, setFilter] = React.useState('') const [open, setOpen] = React.useState(false) @@ -27,6 +32,7 @@ function SimpleSelectPanel(): JSX.Element { onFilterChange={setFilter} open={open} onOpenChange={setOpen} + overlayProps={{width: 'medium', height: 'medium'}} />
@@ -36,10 +42,18 @@ function SimpleSelectPanel(): JSX.Element { } describe('SelectPanel', () => { + const originalScrollTo = Element.prototype.scrollTo + beforeAll(() => { + Element.prototype.scrollTo = () => {} + }) afterEach(() => { jest.clearAllMocks() }) + afterAll(() => { + Element.prototype.scrollTo = originalScrollTo + }) + behavesAsComponent({ Component: SelectPanel, options: {skipAs: true, skipSx: true}, @@ -56,4 +70,20 @@ describe('SelectPanel', () => { const results = await axe(container) expect(results).toHaveNoViolations() }) + + it('should render selected items', () => { + const item = HTMLRender() + const {container} = item + const button = getByRole(container, 'button') + expect(button).toBeTruthy() + + fireEvent.click(button) + const selector = getByText(container, 'Foo') + expect(selector).toBeVisible() + + fireEvent.click(selector) + + const selected = getByRole(container, 'option', {selected: true}) + expect(selected).toBeTruthy() + }) }) diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 6dae84e7f8a..6cf3fbdba62 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -124,9 +124,24 @@ export function SelectPanel({ } }, [placeholder, renderAnchor, selected]) + const testIsItemSelected = React.useCallback((selectedItem: ItemInput, item: ItemInput) => { + const itemType = typeof selectedItem + if (itemType === 'object') { + if (selectedItem.hasOwnProperty('id')) { + return selectedItem.id === item.id + } + } + + return selectedItem === item + }, []) + const itemsToRender = useMemo(() => { return items.map(item => { - const isItemSelected = isMultiSelectVariant(selected) ? selected.includes(item) : selected === item + const isItemSelected = isMultiSelectVariant(selected) + ? selected.some(selectedItem => { + return testIsItemSelected(selectedItem, item) + }) + : selected === item return { ...item, @@ -140,8 +155,13 @@ export function SelectPanel({ } if (isMultiSelectVariant(selected)) { - const otherSelectedItems = selected.filter(selectedItem => selectedItem !== item) - const newSelectedItems = selected.includes(item) ? otherSelectedItems : [...otherSelectedItems, item] + const wasPreviouslySelected = selected.some(selectedItem => { + return testIsItemSelected(selectedItem, item) + }) + const otherSelectedItems = selected.filter(selectedItem => { + return !testIsItemSelected(selectedItem, item) + }) + const newSelectedItems = wasPreviouslySelected ? otherSelectedItems : [...otherSelectedItems, item] const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange'] multiSelectOnChange(newSelectedItems) @@ -155,7 +175,7 @@ export function SelectPanel({ }, } as ItemProps }) - }, [onClose, onSelectedChange, items, selected]) + }, [onClose, onSelectedChange, items, selected, testIsItemSelected]) const inputRef = React.useRef(null) const focusTrapSettings = { From 82af2f0cc1ffd5c5dcb53c383db1a73b5e30fb7b Mon Sep 17 00:00:00 2001 From: Ian Candy Date: Tue, 27 Feb 2024 22:01:53 +0000 Subject: [PATCH 2/3] Allow non-memoized item lists Towards https://github.com/primer/react/issues/4315 The SelectPanel only did a basic equality check for the item state, meaning that it depended on having the exact same objects on multiple pass throughs. This isn't always possible as sometimes you may want to have different objects. This replaces the equality check with a test for an `id` property on the object. If the `id` property isn't present, we fallback to the old behavior. Note that a previous version used the `key` prop, but we decided `id` was a better interface. https://github.com/ipc103/primer-react/commit/f472da25d4f1ff48326c0056aa1696c9c94b4f81#r139163795 Co-authored-by: Andrew Henry --- .changeset/violet-yaks-lie.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/violet-yaks-lie.md diff --git a/.changeset/violet-yaks-lie.md b/.changeset/violet-yaks-lie.md new file mode 100644 index 00000000000..f0cdd470bc3 --- /dev/null +++ b/.changeset/violet-yaks-lie.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Fixes a bug in `SelectPanel` when the list of items was not memoized. From f916ed6200424c7c13c4936e4c7e6383d24cc9b6 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 29 Feb 2024 16:45:20 +0100 Subject: [PATCH 3/3] Update violet-yaks-lie.md --- .changeset/violet-yaks-lie.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/violet-yaks-lie.md b/.changeset/violet-yaks-lie.md index f0cdd470bc3..c17720f1334 100644 --- a/.changeset/violet-yaks-lie.md +++ b/.changeset/violet-yaks-lie.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -Fixes a bug in `SelectPanel` when the list of items was not memoized. +SelectPanel: Fixes a bug in `SelectPanel` when the list of items was not memoized.