From 4d2efffb43b9ea9e75aff5a056e7b9f1d4d56641 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 19 Jul 2023 12:56:28 +0000 Subject: [PATCH 1/4] feat: allSelectedRows and someSelectedRows should be more reliable We took the decision to decouple the selection state from the data. This means that the selection state does not always stay in sync with the data, but the resulting output should be correct. In this case `allSeletedRows` and `someSelectedRows` were calculated based on the size of the selected rows which was wrong, because there can be outdated values in there. Updates the `useTableSelection` hook to make sure that those states reflect the what is in the data. This will cause extra computation (which is memoized) but there is no other way to maintain consistency. It might be possible to update `useSelection` accept the set of selectable items, but the extra computation will still need to be there. Fixes #28456 --- .../src/hooks/useTableSelection.test.ts | 56 +++++ .../src/hooks/useTableSelection.ts | 49 +++- .../stories/Table/Default.stories.tsx | 209 +++++++++++++----- 3 files changed, 259 insertions(+), 55 deletions(-) diff --git a/packages/react-components/react-table/src/hooks/useTableSelection.test.ts b/packages/react-components/react-table/src/hooks/useTableSelection.test.ts index f0cf882f0c043a..39a97d4254f03d 100644 --- a/packages/react-components/react-table/src/hooks/useTableSelection.test.ts +++ b/packages/react-components/react-table/src/hooks/useTableSelection.test.ts @@ -219,6 +219,36 @@ describe('useTableSelectionState', () => { }); describe('allRowsSelected', () => { + it('should return true after items updated if all selectable rows are selected', () => { + const getRowId = (item: { value: string }) => item.value; + let tableState = mockTableState({ items, getRowId }); + const { result, rerender } = renderHook(() => + useTableSelectionState(tableState, { selectionMode: 'multiselect' }), + ); + + act(() => { + result.current.selection.toggleAllRows(mockSyntheticEvent()); + }); + + act(() => { + result.current.selection.deselectRow(mockSyntheticEvent(), 'c'); + }); + + expect(result.current.selection.allRowsSelected).toBe(false); + + // remove the deselected item + const nextItems = [...items]; + const indexToDelete = nextItems.findIndex(x => x.value === 'c'); + nextItems.splice(indexToDelete, 1); + tableState = mockTableState({ items: nextItems, getRowId }); + + act(() => { + rerender(); + }); + + expect(result.current.selection.allRowsSelected).toBe(true); + }); + it('should return true if all rows are selected', () => { const { result } = renderHook(() => useTableSelectionState(mockTableState({ items }), { selectionMode: 'multiselect' }), @@ -258,6 +288,32 @@ describe('useTableSelectionState', () => { }); describe('someRowsSelected', () => { + it('should return false after selectedItems are removed', () => { + const getRowId = (item: { value: string }) => item.value; + let tableState = mockTableState({ items, getRowId }); + const { result, rerender } = renderHook(() => + useTableSelectionState(tableState, { selectionMode: 'multiselect' }), + ); + + act(() => { + result.current.selection.selectRow(mockSyntheticEvent(), 'a'); + }); + + expect(result.current.selection.someRowsSelected).toBe(true); + + // remove the deselected item + const nextItems = [...items]; + const indexToDelete = nextItems.findIndex(x => x.value === 'a'); + nextItems.splice(indexToDelete, 1); + tableState = mockTableState({ items: nextItems, getRowId }); + + act(() => { + rerender(); + }); + + expect(result.current.selection.someRowsSelected).toBe(false); + }); + it('should return true if there is a selected row', () => { const { result } = renderHook(() => useTableSelectionState(mockTableState({ items }), { selectionMode: 'multiselect' }), diff --git a/packages/react-components/react-table/src/hooks/useTableSelection.ts b/packages/react-components/react-table/src/hooks/useTableSelection.ts index 659b32cae8a563..b853c0201744fb 100644 --- a/packages/react-components/react-table/src/hooks/useTableSelection.ts +++ b/packages/react-components/react-table/src/hooks/useTableSelection.ts @@ -1,3 +1,4 @@ +import * as React from 'react'; import { SelectionHookParams, useEventCallback, useSelection } from '@fluentui/react-utilities'; import type { TableRowId, TableSelectionState, TableFeaturesState } from './types'; @@ -36,6 +37,50 @@ export function useTableSelectionState( onSelectionChange, }); + // Selection state can contain obselete items (i.e. rows that are removed) + const selectableRowIds = React.useMemo(() => { + const rowIds = new Set(); + for (let i = 0; i < items.length; i++) { + rowIds.add(getRowId?.(items[i]) ?? i); + } + + return rowIds; + }, [items, getRowId]); + + const allRowsSelected = React.useMemo(() => { + if (selectionMode === 'single') { + const selectedRow = Array.from(selected)[0]; + return selectableRowIds.has(selectedRow); + } + + // multiselect case + if (selected.size < selectableRowIds.size) { + return false; + } + + for (const selectableRowId of selectableRowIds) { + if (!selected.has(selectableRowId)) { + return false; + } + } + + return true; + }, [selectableRowIds, selected, selectionMode]); + + const someRowsSelected = React.useMemo(() => { + if (selected.size <= 0) { + return false; + } + + for (const selectableRowId of selectableRowIds) { + if (selected.has(selectableRowId)) { + return true; + } + } + + return false; + }, [selectableRowIds, selected]); + const toggleAllRows: TableSelectionState['toggleAllRows'] = useEventCallback(e => { selectionMethods.toggleAllItems( e, @@ -63,8 +108,8 @@ export function useTableSelectionState( ...tableState, selection: { selectionMode, - someRowsSelected: selected.size > 0, - allRowsSelected: selectionMode === 'single' ? selected.size > 0 : selected.size === items.length, + someRowsSelected, + allRowsSelected, selectedRows: selected, toggleRow, toggleAllRows, diff --git a/packages/react-components/react-table/stories/Table/Default.stories.tsx b/packages/react-components/react-table/stories/Table/Default.stories.tsx index 78daab0dd384b4..2ad172159161f8 100644 --- a/packages/react-components/react-table/stories/Table/Default.stories.tsx +++ b/packages/react-components/react-table/stories/Table/Default.stories.tsx @@ -1,3 +1,20 @@ +import { + PresenceBadgeStatus, + Avatar, + TableBody, + TableCell, + TableRow, + Table, + TableHeader, + TableHeaderCell, + TableSelectionCell, + TableCellLayout, + useTableFeatures, + TableColumnDefinition, + useTableSelection, + createTableColumn, + Button, +} from '@fluentui/react-components'; import * as React from 'react'; import { FolderRegular, @@ -8,23 +25,39 @@ import { DocumentPdfRegular, VideoRegular, } from '@fluentui/react-icons'; -import { - TableBody, - TableCell, - TableRow, - Table, - TableHeader, - TableHeaderCell, - TableCellLayout, - PresenceBadgeStatus, - Avatar, -} from '@fluentui/react-components'; -const items = [ +type FileCell = { + label: string; + icon: JSX.Element; +}; + +type LastUpdatedCell = { + label: string; + timestamp: number; +}; + +type LastUpdateCell = { + label: string; + icon: JSX.Element; +}; + +type AuthorCell = { + label: string; + status: PresenceBadgeStatus; +}; + +type Item = { + file: FileCell; + author: AuthorCell; + lastUpdated: LastUpdatedCell; + lastUpdate: LastUpdateCell; +}; + +let data: Item[] = [ { file: { label: 'Meeting notes', icon: }, author: { label: 'Max Mustermann', status: 'available' }, - lastUpdated: { label: '7h ago', timestamp: 1 }, + lastUpdated: { label: '7h ago', timestamp: 3 }, lastUpdate: { label: 'You edited this', icon: , @@ -51,7 +84,7 @@ const items = [ { file: { label: 'Purchase order', icon: }, author: { label: 'Jane Doe', status: 'offline' }, - lastUpdated: { label: 'Tue at 9:30 AM', timestamp: 3 }, + lastUpdated: { label: 'Tue at 9:30 AM', timestamp: 1 }, lastUpdate: { label: 'You shared this in a Teams chat', icon: , @@ -59,49 +92,119 @@ const items = [ }, ]; -const columns = [ - { columnKey: 'file', label: 'File' }, - { columnKey: 'author', label: 'Author' }, - { columnKey: 'lastUpdated', label: 'Last updated' }, - { columnKey: 'lastUpdate', label: 'Last update' }, +const columns: TableColumnDefinition[] = [ + createTableColumn({ + columnId: 'file', + }), + createTableColumn({ + columnId: 'author', + }), + createTableColumn({ + columnId: 'lastUpdated', + }), + createTableColumn({ + columnId: 'lastUpdate', + }), ]; export const Default = () => { + const [items, setItems] = React.useState(data); + + const { + getRows, + selection: { allRowsSelected, someRowsSelected, toggleAllRows, toggleRow, isRowSelected }, + } = useTableFeatures( + { + columns, + items, + getRowId: item => item.file.label, + }, + [ + useTableSelection({ + selectionMode: 'multiselect', + defaultSelectedItems: new Set([0, 1]), + }), + ], + ); + + let rows = getRows(row => { + const selected = isRowSelected(row.rowId); + return { + ...row, + onClick: (e: React.MouseEvent) => toggleRow(e, row.rowId), + onKeyDown: (e: React.KeyboardEvent) => { + if (e.key === ' ') { + e.preventDefault(); + toggleRow(e, row.rowId); + } + }, + selected, + appearance: selected ? ('brand' as const) : ('none' as const), + }; + }); + + const toggleAllKeydown = React.useCallback( + (e: React.KeyboardEvent) => { + if (e.key === ' ') { + toggleAllRows(e); + e.preventDefault(); + } + }, + [toggleAllRows], + ); + + const onDelete = () => { + data = data.filter((x, index) => index !== 0); + setItems(data); + }; + return ( - - - - {columns.map(column => ( - {column.label} - ))} - - - - {items.map(item => ( - - - {item.file.label} - - - - } - > - {item.author.label} - - - {item.lastUpdated.label} - - {item.lastUpdate.label} - + <> + +
+ + + + + File + Author + Last updated + Last update - ))} - -
+ + + {rows.map(({ item, selected, onClick, onKeyDown, appearance }) => ( + + + + {item.file.label} + + + } + > + {item.author.label} + + + {item.lastUpdated.label} + + {item.lastUpdate.label} + + + ))} + + + ); }; From ea3f5c09faf9825a1b56533bb6ebaf9060911c6a Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 19 Jul 2023 12:59:49 +0000 Subject: [PATCH 2/4] changefile --- ...i-react-table-9bafa4eb-c107-4426-b422-56d0113f018b.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-table-9bafa4eb-c107-4426-b422-56d0113f018b.json diff --git a/change/@fluentui-react-table-9bafa4eb-c107-4426-b422-56d0113f018b.json b/change/@fluentui-react-table-9bafa4eb-c107-4426-b422-56d0113f018b.json new file mode 100644 index 00000000000000..42bbd5b511f14f --- /dev/null +++ b/change/@fluentui-react-table-9bafa4eb-c107-4426-b422-56d0113f018b.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "28456", + "packageName": "@fluentui/react-table", + "email": "lingfan.gao@microsoft.com", + "dependentChangeType": "patch" +} From cf9f870aac8e9c065bf156cff377264019f6684f Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 19 Jul 2023 13:45:53 +0000 Subject: [PATCH 3/4] don't use set iteration --- .../react-table/src/hooks/useTableSelection.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/react-components/react-table/src/hooks/useTableSelection.ts b/packages/react-components/react-table/src/hooks/useTableSelection.ts index b853c0201744fb..7f2b94bf8de82e 100644 --- a/packages/react-components/react-table/src/hooks/useTableSelection.ts +++ b/packages/react-components/react-table/src/hooks/useTableSelection.ts @@ -58,13 +58,14 @@ export function useTableSelectionState( return false; } - for (const selectableRowId of selectableRowIds) { + let res = true; + selectableRowIds.forEach(selectableRowId => { if (!selected.has(selectableRowId)) { - return false; + res = false; } - } + }); - return true; + return res; }, [selectableRowIds, selected, selectionMode]); const someRowsSelected = React.useMemo(() => { @@ -72,13 +73,14 @@ export function useTableSelectionState( return false; } - for (const selectableRowId of selectableRowIds) { + let res = false; + selectableRowIds.forEach(selectableRowId => { if (selected.has(selectableRowId)) { - return true; + res = true; } - } + }); - return false; + return res; }, [selectableRowIds, selected]); const toggleAllRows: TableSelectionState['toggleAllRows'] = useEventCallback(e => { From 6da7bb16209f42d216b69ad35865177dcdb8200f Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 20 Jul 2023 16:16:47 +0000 Subject: [PATCH 4/4] revert --- .../stories/Table/Default.stories.tsx | 209 +++++------------- 1 file changed, 53 insertions(+), 156 deletions(-) diff --git a/packages/react-components/react-table/stories/Table/Default.stories.tsx b/packages/react-components/react-table/stories/Table/Default.stories.tsx index 2ad172159161f8..78daab0dd384b4 100644 --- a/packages/react-components/react-table/stories/Table/Default.stories.tsx +++ b/packages/react-components/react-table/stories/Table/Default.stories.tsx @@ -1,20 +1,3 @@ -import { - PresenceBadgeStatus, - Avatar, - TableBody, - TableCell, - TableRow, - Table, - TableHeader, - TableHeaderCell, - TableSelectionCell, - TableCellLayout, - useTableFeatures, - TableColumnDefinition, - useTableSelection, - createTableColumn, - Button, -} from '@fluentui/react-components'; import * as React from 'react'; import { FolderRegular, @@ -25,39 +8,23 @@ import { DocumentPdfRegular, VideoRegular, } from '@fluentui/react-icons'; +import { + TableBody, + TableCell, + TableRow, + Table, + TableHeader, + TableHeaderCell, + TableCellLayout, + PresenceBadgeStatus, + Avatar, +} from '@fluentui/react-components'; -type FileCell = { - label: string; - icon: JSX.Element; -}; - -type LastUpdatedCell = { - label: string; - timestamp: number; -}; - -type LastUpdateCell = { - label: string; - icon: JSX.Element; -}; - -type AuthorCell = { - label: string; - status: PresenceBadgeStatus; -}; - -type Item = { - file: FileCell; - author: AuthorCell; - lastUpdated: LastUpdatedCell; - lastUpdate: LastUpdateCell; -}; - -let data: Item[] = [ +const items = [ { file: { label: 'Meeting notes', icon: }, author: { label: 'Max Mustermann', status: 'available' }, - lastUpdated: { label: '7h ago', timestamp: 3 }, + lastUpdated: { label: '7h ago', timestamp: 1 }, lastUpdate: { label: 'You edited this', icon: , @@ -84,7 +51,7 @@ let data: Item[] = [ { file: { label: 'Purchase order', icon: }, author: { label: 'Jane Doe', status: 'offline' }, - lastUpdated: { label: 'Tue at 9:30 AM', timestamp: 1 }, + lastUpdated: { label: 'Tue at 9:30 AM', timestamp: 3 }, lastUpdate: { label: 'You shared this in a Teams chat', icon: , @@ -92,119 +59,49 @@ let data: Item[] = [ }, ]; -const columns: TableColumnDefinition[] = [ - createTableColumn({ - columnId: 'file', - }), - createTableColumn({ - columnId: 'author', - }), - createTableColumn({ - columnId: 'lastUpdated', - }), - createTableColumn({ - columnId: 'lastUpdate', - }), +const columns = [ + { columnKey: 'file', label: 'File' }, + { columnKey: 'author', label: 'Author' }, + { columnKey: 'lastUpdated', label: 'Last updated' }, + { columnKey: 'lastUpdate', label: 'Last update' }, ]; export const Default = () => { - const [items, setItems] = React.useState(data); - - const { - getRows, - selection: { allRowsSelected, someRowsSelected, toggleAllRows, toggleRow, isRowSelected }, - } = useTableFeatures( - { - columns, - items, - getRowId: item => item.file.label, - }, - [ - useTableSelection({ - selectionMode: 'multiselect', - defaultSelectedItems: new Set([0, 1]), - }), - ], - ); - - let rows = getRows(row => { - const selected = isRowSelected(row.rowId); - return { - ...row, - onClick: (e: React.MouseEvent) => toggleRow(e, row.rowId), - onKeyDown: (e: React.KeyboardEvent) => { - if (e.key === ' ') { - e.preventDefault(); - toggleRow(e, row.rowId); - } - }, - selected, - appearance: selected ? ('brand' as const) : ('none' as const), - }; - }); - - const toggleAllKeydown = React.useCallback( - (e: React.KeyboardEvent) => { - if (e.key === ' ') { - toggleAllRows(e); - e.preventDefault(); - } - }, - [toggleAllRows], - ); - - const onDelete = () => { - data = data.filter((x, index) => index !== 0); - setItems(data); - }; - return ( - <> - - - - - - - File - Author - Last updated - Last update - - - - {rows.map(({ item, selected, onClick, onKeyDown, appearance }) => ( - - - - {item.file.label} - - - } - > - {item.author.label} - - - {item.lastUpdated.label} - - {item.lastUpdate.label} - - +
+ + + {columns.map(column => ( + {column.label} ))} - -
- + + + + {items.map(item => ( + + + {item.file.label} + + + + } + > + {item.author.label} + + + {item.lastUpdated.label} + + {item.lastUpdate.label} + + + ))} + + ); };