From 160fc85e52142e1fff1fd2a917398f50aab6ff81 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 7 Oct 2024 13:00:26 +0200 Subject: [PATCH 1/3] add failing test --- .../src/SelectPanel/SelectPanel.test.tsx | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index 936f3b544fc..f485cea23ea 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -335,6 +335,48 @@ for (const useModernActionList of [false, true]) { screen.getByRole('option', {name: 'item one'}).id, ) }) + + it('should select an item (by item.id) even when items are defined in the component', async () => { + const user = userEvent.setup() + + function Fixture() { + // items are defined in the same scope as selection, so they could rerender and create new object references + const items: SelectPanelProps['items'] = [{text: 'item one'}, {text: 'item two'}, {text: 'item three'}] + + const [open, setOpen] = React.useState(false) + const [selected, setSelected] = React.useState([]) + const [filter, setFilter] = React.useState('') + + return ( + + + + ) + } + + renderWithFlag(, useModernActionList) + + await user.click(screen.getByText('Select items')) + + await user.click(screen.getByText('item one')) + expect(screen.getByRole('option', {name: 'item one'})).toHaveAttribute('aria-selected', 'true') + + await user.click(screen.getByText('item two')) + expect(screen.getByRole('option', {name: 'item two'})).toHaveAttribute('aria-selected', 'true') + + await user.click(screen.getByRole('option', {name: 'item one'})) + expect(screen.getByRole('option', {name: 'item one'})).toHaveAttribute('aria-selected', 'false') + }) }) function FilterableSelectPanel() { From c8154f65ce3ed8eb47f27480536388981ee1f919 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 7 Oct 2024 13:00:52 +0200 Subject: [PATCH 2/3] track items by id if possible --- .../SelectPanel.examples.stories.tsx | 36 +++++++++++++++++++ .../src/SelectPanel/SelectPanel.test.tsx | 7 +++- .../react/src/SelectPanel/SelectPanel.tsx | 18 ++++++++-- 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx b/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx index 248432e0f9a..3e261c6bce3 100644 --- a/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx @@ -309,3 +309,39 @@ export const CustomItemRenderer = () => { ) } + +export const ItemsInScope = () => { + // items are defined in the same scope as selection, so they could rerender and create new object references + // We use item.id to track selection + // Reported in: https://github.com/primer/react/issues/4315 + const items = [ + {text: 'enhancement', id: 1}, + {text: 'bug', id: 2}, + {text: 'good first issue', id: 3}, + {text: 'design', id: 4}, + {text: 'blocker', id: 5}, + {text: 'backend', id: 6}, + {text: 'frontend', id: 7}, + ] + + const [selected, setSelected] = React.useState([items[0], items[1]]) + const [open, setOpen] = useState(false) + const [filter, setFilter] = React.useState('') + const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) + + return ( + <> +

Items in component scope

+ + + ) +} diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index f485cea23ea..a047507fb7e 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -341,7 +341,12 @@ for (const useModernActionList of [false, true]) { function Fixture() { // items are defined in the same scope as selection, so they could rerender and create new object references - const items: SelectPanelProps['items'] = [{text: 'item one'}, {text: 'item two'}, {text: 'item three'}] + // We use item.id to track selection + const items: SelectPanelProps['items'] = [ + {id: 'one', text: 'item one'}, + {id: 'two', text: 'item two'}, + {id: 'three', text: 'item three'}, + ] const [open, setOpen] = React.useState(false) const [selected, setSelected] = React.useState([]) diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index efae1b5a7e1..c862950deb3 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -61,6 +61,16 @@ const focusZoneSettings: Partial = { disabled: true, } +const areItemsEqual = (itemA: ItemInput, itemB: ItemInput) => { + // prefer checking equivality by item.id + if (typeof itemA.id !== 'undefined') return itemA.id === itemB.id + else return itemA === itemB +} + +const doesItemsIncludeItem = (items: ItemInput[], item: ItemInput) => { + return items.some(i => areItemsEqual(i, item)) +} + export function SelectPanel({ open, onOpenChange, @@ -129,7 +139,7 @@ export function SelectPanel({ const itemsToRender = useMemo(() => { return items.map(item => { - const isItemSelected = isMultiSelectVariant(selected) ? selected.includes(item) : selected === item + const isItemSelected = isMultiSelectVariant(selected) ? doesItemsIncludeItem(selected, item) : selected === item return { ...item, @@ -143,8 +153,10 @@ export function SelectPanel({ } if (isMultiSelectVariant(selected)) { - const otherSelectedItems = selected.filter(selectedItem => selectedItem !== item) - const newSelectedItems = selected.includes(item) ? otherSelectedItems : [...otherSelectedItems, item] + const otherSelectedItems = selected.filter(selectedItem => !areItemsEqual(selectedItem, item)) + const newSelectedItems = doesItemsIncludeItem(selected, item) + ? otherSelectedItems + : [...otherSelectedItems, item] const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange'] multiSelectOnChange(newSelectedItems) From fc23f0dc3627bd967a5dfbc376798ef1f48016b0 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 7 Oct 2024 13:02:43 +0200 Subject: [PATCH 3/3] Create stupid-monkeys-beg.md --- .changeset/stupid-monkeys-beg.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/stupid-monkeys-beg.md diff --git a/.changeset/stupid-monkeys-beg.md b/.changeset/stupid-monkeys-beg.md new file mode 100644 index 00000000000..685c96bab90 --- /dev/null +++ b/.changeset/stupid-monkeys-beg.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +SelectPanel: Fix items not being selected when defined within scope (track selection by item.id)