From 9f60779816a0772cd21f1a61847bf938d7274c37 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 16 Aug 2022 12:53:32 +0200 Subject: [PATCH 1/6] feat(useTable): selection manager to avoid calling multiple hooks Implements a `createSelectionManager` function to manage selection state independently of react and avoids calling state hooks twice on each render --- .../react-table/etc/react-table.api.md | 2 +- .../react-table/src/hooks/selectionManager.ts | 112 ++++++ .../react-table/src/hooks/types.ts | 3 +- .../src/hooks/useMultipleSelection.test.ts | 199 ---------- .../src/hooks/useMultipleSelection.ts | 83 ---- .../src/hooks/useSelection.test.ts | 356 ++++++++++++++++++ .../react-table/src/hooks/useSelection.ts | 38 +- .../src/hooks/useSingleSelection.test.ts | 157 -------- .../src/hooks/useSingleSelection.ts | 32 -- 9 files changed, 502 insertions(+), 480 deletions(-) create mode 100644 packages/react-components/react-table/src/hooks/selectionManager.ts delete mode 100644 packages/react-components/react-table/src/hooks/useMultipleSelection.test.ts delete mode 100644 packages/react-components/react-table/src/hooks/useMultipleSelection.ts create mode 100644 packages/react-components/react-table/src/hooks/useSelection.test.ts delete mode 100644 packages/react-components/react-table/src/hooks/useSingleSelection.test.ts delete mode 100644 packages/react-components/react-table/src/hooks/useSingleSelection.ts diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index 1f17dc618949fa..f06f1a8b2f9538 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -349,7 +349,7 @@ export interface UseTableOptions = RowS // (undocumented) rowEnhancer?: RowEnhancer; // (undocumented) - selectionMode?: 'single' | 'multiselect'; + selectionMode?: SelectionMode_2; } // @public diff --git a/packages/react-components/react-table/src/hooks/selectionManager.ts b/packages/react-components/react-table/src/hooks/selectionManager.ts new file mode 100644 index 00000000000000..84774f1920010b --- /dev/null +++ b/packages/react-components/react-table/src/hooks/selectionManager.ts @@ -0,0 +1,112 @@ +import { SelectionMode } from './types'; + +type OnSelectionChangeCallback = (selectedItems: Set) => void; + +export interface SelectionManager { + toggleSelect(id: SelectionItemId): void; + select(id: SelectionItemId): void; + deSelect(id: SelectionItemId): void; + clearSelection(): void; + isSelected(id: SelectionItemId): boolean; + toggleSelectAll(itemIds: SelectionItemId[]): void; +} + +export type SelectionItemId = string | number; + +export function createSelectionManager( + mode: SelectionMode, + onSelectionChange: OnSelectionChangeCallback = () => undefined, +): SelectionManager { + const selectedItems = new Set(); + return mode === 'multiselect' + ? createMultipleSelectionManager(selectedItems, onSelectionChange) + : createSingleSelectionManager(selectedItems, onSelectionChange); +} + +function createMultipleSelectionManager( + set: Set, + onSelectionChange: OnSelectionChangeCallback, +): SelectionManager { + const toggleSelectAll = (itemIds: SelectionItemId[]) => { + if (set.size === itemIds.length) { + set.clear(); + } else { + itemIds.forEach(itemId => set.add(itemId)); + } + + onSelectionChange(new Set(set)); + }; + + const toggleSelect = (itemId: SelectionItemId) => { + if (set.has(itemId)) { + set.delete(itemId); + } else { + set.add(itemId); + } + + onSelectionChange(new Set(set)); + }; + + const select = (itemId: SelectionItemId) => { + set.add(itemId); + onSelectionChange(new Set(set)); + }; + + const deSelect = (itemId: SelectionItemId) => { + set.delete(itemId); + onSelectionChange(new Set(set)); + }; + + const clearSelection = () => { + set.clear(); + onSelectionChange(new Set(set)); + }; + + const isSelected = (itemId: SelectionItemId) => { + return set.has(itemId); + }; + + return { + toggleSelect, + select, + deSelect, + clearSelection, + isSelected, + toggleSelectAll, + }; +} + +function createSingleSelectionManager( + set: Set, + onSelectionChange: OnSelectionChangeCallback, +): SelectionManager { + const toggleSelect = (itemId: SelectionItemId) => { + set.clear(); + set.add(itemId); + onSelectionChange(new Set(set)); + }; + + const clearSelection = () => { + set.clear(); + onSelectionChange(new Set(set)); + }; + + const isSelected = (itemId: SelectionItemId) => { + return set.has(itemId); + }; + + const select = (itemId: SelectionItemId) => { + set.clear(); + set.add(itemId); + onSelectionChange(new Set(set)); + }; + + return { + deSelect: clearSelection, + select, + toggleSelectAll: () => undefined, + toggleSelect, + clearSelection, + isSelected, + }; +} diff --git a/packages/react-components/react-table/src/hooks/types.ts b/packages/react-components/react-table/src/hooks/types.ts index 8bd90f8929d84f..c677c2c23008b5 100644 --- a/packages/react-components/react-table/src/hooks/types.ts +++ b/packages/react-components/react-table/src/hooks/types.ts @@ -3,6 +3,7 @@ import { SortDirection } from '../components/Table/Table.types'; export type RowId = string | number; export type ColumnId = string | number; export type GetRowIdInternal = (rowId: TItem, index: number) => RowId; +export type SelectionMode = 'single' | 'multiselect'; export interface ColumnDefinition { columnId: ColumnId; @@ -29,7 +30,7 @@ export interface SortStateInternal { export interface UseTableOptions = RowState> { columns: ColumnDefinition[]; items: TItem[]; - selectionMode?: 'single' | 'multiselect'; + selectionMode?: SelectionMode; getRowId?: (item: TItem) => RowId; rowEnhancer?: RowEnhancer; } diff --git a/packages/react-components/react-table/src/hooks/useMultipleSelection.test.ts b/packages/react-components/react-table/src/hooks/useMultipleSelection.test.ts deleted file mode 100644 index 70125f694b502c..00000000000000 --- a/packages/react-components/react-table/src/hooks/useMultipleSelection.test.ts +++ /dev/null @@ -1,199 +0,0 @@ -import { renderHook, act } from '@testing-library/react-hooks'; -import { useMultipleSelection } from './useMultipleSelection'; - -describe('useMultipleSelection', () => { - const items = [{ value: 'a' }, { value: 'b' }, { value: 'c' }, { value: 'd' }]; - - const getRowId = (item: {}, index: number) => index; - - it('should use custom row id', () => { - const { result } = renderHook(() => useMultipleSelection(items, (item: { value: string }) => item.value)); - act(() => { - result.current.toggleSelectAllRows(); - }); - - expect(Array.from(result.current.selectedRows)).toEqual(items.map(item => item.value)); - }); - - describe('toggleSelectAllRows', () => { - it('should select all rows', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleSelectAllRows(); - }); - expect(result.current.selectedRows.size).toBe(items.length); - expect(Array.from(result.current.selectedRows)).toEqual(items.map((_, i) => i)); - }); - - it('should deSelect all rows', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleSelectAllRows(); - }); - - act(() => { - result.current.toggleSelectAllRows(); - }); - - expect(result.current.selectedRows.size).toBe(0); - }); - }); - describe('clearSelection', () => { - it('should clear selection', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleSelectAllRows(); - }); - act(() => { - result.current.clearSelection(); - }); - - expect(result.current.selectedRows.size).toBe(0); - }); - }); - - describe('selectRow', () => { - it('should select row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.selectRow(1); - }); - - expect(result.current.selectedRows.has(1)).toBe(true); - }); - - it('should select multiple rows', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.selectRow(1); - }); - - act(() => { - result.current.selectRow(2); - }); - - expect(result.current.selectedRows.size).toBe(2); - expect(result.current.selectedRows.has(1)).toBe(true); - expect(result.current.selectedRows.has(2)).toBe(true); - }); - }); - - describe('deSelectRow', () => { - it('should make row unselected', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.selectRow(1); - }); - - act(() => { - result.current.deSelectRow(1); - }); - - expect(result.current.selectedRows.size).toBe(0); - }); - }); - - describe('toggleRowSelect', () => { - it('should select unselected row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleRowSelect(1); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.selectedRows.has(1)).toBe(true); - }); - - it('should deselect selected row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleRowSelect(1); - }); - - act(() => { - result.current.toggleRowSelect(1); - }); - - expect(result.current.selectedRows.size).toBe(0); - expect(result.current.selectedRows.has(1)).toBe(false); - }); - - it('should select another unselected row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleRowSelect(1); - }); - - act(() => { - result.current.toggleRowSelect(2); - }); - - expect(result.current.selectedRows.size).toBe(2); - expect(result.current.selectedRows.has(1)).toBe(true); - expect(result.current.selectedRows.has(2)).toBe(true); - }); - }); - - describe('allRowsSelected', () => { - it('should return true if all rows are selected', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleSelectAllRows(); - }); - - expect(result.current.allRowsSelected).toBe(true); - }); - - it('should return false if there is no selected row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - expect(result.current.selectedRows.size).toBe(0); - expect(result.current.allRowsSelected).toBe(false); - }); - - it('should return false if not all rows are selected', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.toggleSelectAllRows(); - }); - - act(() => { - result.current.deSelectRow(1); - }); - - expect(result.current.selectedRows.size).toBe(3); - expect(result.current.allRowsSelected).toBe(false); - }); - }); - - describe('someRowsSelected', () => { - it('should return true if there is a selected row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - act(() => { - result.current.selectRow(1); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.someRowsSelected).toBe(true); - }); - - it('should return false if there is no selected row', () => { - const { result } = renderHook(() => useMultipleSelection(items, getRowId)); - - expect(result.current.selectedRows.size).toBe(0); - expect(result.current.someRowsSelected).toBe(false); - }); - }); -}); diff --git a/packages/react-components/react-table/src/hooks/useMultipleSelection.ts b/packages/react-components/react-table/src/hooks/useMultipleSelection.ts deleted file mode 100644 index e38df8401d7fa7..00000000000000 --- a/packages/react-components/react-table/src/hooks/useMultipleSelection.ts +++ /dev/null @@ -1,83 +0,0 @@ -import * as React from 'react'; -import { useEventCallback } from '@fluentui/react-utilities'; -import type { SelectionStateInternal, RowId, GetRowIdInternal } from './types'; - -export function useMultipleSelection(items: TItem[], getRowId: GetRowIdInternal): SelectionStateInternal { - const [selected, setSelected] = React.useState(new Set()); - const allRowIds = React.useMemo(() => new Set(items.map((item, i) => getRowId(item, i))), [items, getRowId]); - - const toggleSelectAllRows: SelectionStateInternal['toggleSelectAllRows'] = useEventCallback(() => { - setSelected(s => { - if (s.size === items.length) { - return new Set(); - } - - return new Set(items.map((item, i) => getRowId(item, i))); - }); - }); - - const toggleRowSelect: SelectionStateInternal['toggleRowSelect'] = React.useCallback( - (rowId: RowId) => { - if (!allRowIds.has(rowId)) { - return; - } - - setSelected(s => { - const newState = new Set(s); - if (newState.has(rowId)) { - newState.delete(rowId); - } else { - newState.add(rowId); - } - - return newState; - }); - }, - [allRowIds], - ); - - const selectRow: SelectionStateInternal['selectRow'] = React.useCallback( - (rowId: RowId) => { - if (!allRowIds.has(rowId)) { - return; - } - setSelected(s => { - const newState = new Set(s); - newState.add(rowId); - return newState; - }); - }, - [allRowIds], - ); - - const deSelectRow: SelectionStateInternal['selectRow'] = React.useCallback((rowId: RowId) => { - setSelected(s => { - const newState = new Set(s); - newState.delete(rowId); - return newState; - }); - }, []); - - const clearSelection: SelectionStateInternal['clearSelection'] = React.useCallback(() => { - setSelected(new Set()); - }, []); - - const isRowSelected: SelectionStateInternal['isRowSelected'] = React.useCallback( - (rowId: RowId) => { - return selected.has(rowId); - }, - [selected], - ); - - return { - isRowSelected, - clearSelection, - deSelectRow, - selectRow, - toggleRowSelect, - toggleSelectAllRows, - selectedRows: selected, - allRowsSelected: selected.size === items.length, - someRowsSelected: selected.size > 0, - }; -} diff --git a/packages/react-components/react-table/src/hooks/useSelection.test.ts b/packages/react-components/react-table/src/hooks/useSelection.test.ts new file mode 100644 index 00000000000000..6c88d430e398c9 --- /dev/null +++ b/packages/react-components/react-table/src/hooks/useSelection.test.ts @@ -0,0 +1,356 @@ +import { renderHook, act } from '@testing-library/react-hooks'; +import { useSelection } from './useSelection'; + +describe('useSelection', () => { + const items = [{ value: 'a' }, { value: 'b' }, { value: 'c' }, { value: 'd' }]; + + const getRowId = (item: {}, index: number) => index; + + describe('multiselect', () => { + it('should use custom row id', () => { + const { result } = renderHook(() => useSelection('multiselect', items, (item: { value: string }) => item.value)); + act(() => { + result.current.toggleSelectAllRows(); + }); + + expect(Array.from(result.current.selectedRows)).toEqual(items.map(item => item.value)); + }); + + describe('toggleSelectAllRows', () => { + it('should select all rows', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleSelectAllRows(); + }); + expect(result.current.selectedRows.size).toBe(items.length); + expect(Array.from(result.current.selectedRows)).toEqual(items.map((_, i) => i)); + }); + + it('should deSelect all rows', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleSelectAllRows(); + }); + + act(() => { + result.current.toggleSelectAllRows(); + }); + + expect(result.current.selectedRows.size).toBe(0); + }); + }); + describe('clearSelection', () => { + it('should clear selection', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleSelectAllRows(); + }); + act(() => { + result.current.clearSelection(); + }); + + expect(result.current.selectedRows.size).toBe(0); + }); + }); + + describe('selectRow', () => { + it('should select row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + expect(result.current.selectedRows.has(1)).toBe(true); + }); + + it('should select multiple rows', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + act(() => { + result.current.selectRow(2); + }); + + expect(result.current.selectedRows.size).toBe(2); + expect(result.current.selectedRows.has(1)).toBe(true); + expect(result.current.selectedRows.has(2)).toBe(true); + }); + }); + + describe('deSelectRow', () => { + it('should make row unselected', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + act(() => { + result.current.deSelectRow(1); + }); + + expect(result.current.selectedRows.size).toBe(0); + }); + }); + + describe('toggleRowSelect', () => { + it('should select unselected row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleRowSelect(1); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.selectedRows.has(1)).toBe(true); + }); + + it('should deselect selected row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleRowSelect(1); + }); + + act(() => { + result.current.toggleRowSelect(1); + }); + + expect(result.current.selectedRows.size).toBe(0); + expect(result.current.selectedRows.has(1)).toBe(false); + }); + + it('should select another unselected row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleRowSelect(1); + }); + + act(() => { + result.current.toggleRowSelect(2); + }); + + expect(result.current.selectedRows.size).toBe(2); + expect(result.current.selectedRows.has(1)).toBe(true); + expect(result.current.selectedRows.has(2)).toBe(true); + }); + }); + + describe('allRowsSelected', () => { + it('should return true if all rows are selected', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleSelectAllRows(); + }); + + expect(result.current.allRowsSelected).toBe(true); + }); + + it('should return false if there is no selected row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + expect(result.current.selectedRows.size).toBe(0); + expect(result.current.allRowsSelected).toBe(false); + }); + + it('should return false if not all rows are selected', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.toggleSelectAllRows(); + }); + + act(() => { + result.current.deSelectRow(1); + }); + + expect(result.current.selectedRows.size).toBe(3); + expect(result.current.allRowsSelected).toBe(false); + }); + }); + + describe('someRowsSelected', () => { + it('should return true if there is a selected row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.someRowsSelected).toBe(true); + }); + + it('should return false if there is no selected row', () => { + const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); + + expect(result.current.selectedRows.size).toBe(0); + expect(result.current.someRowsSelected).toBe(false); + }); + }); + }); + + describe('single select', () => { + describe('toggleSelectAllRows', () => { + it('should be a noop', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + result.current.toggleSelectAllRows(); + expect(result.current.selectedRows.size).toBe(0); + + result.current.toggleSelectAllRows(); + expect(result.current.selectedRows.size).toBe(0); + }); + }); + describe('clearSelection', () => { + it('should clear selection', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + act(() => { + result.current.clearSelection(); + }); + + expect(result.current.selectedRows.size).toBe(0); + }); + }); + + describe('selectRow', () => { + it('should select row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + expect(result.current.selectedRows.has(1)).toBe(true); + }); + + it('should select another row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + act(() => { + result.current.selectRow(2); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.selectedRows.has(2)).toBe(true); + }); + }); + + describe('deSelectRow', () => { + it('should make row unselected', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + act(() => { + result.current.deSelectRow(1); + }); + + expect(result.current.selectedRows.size).toBe(0); + }); + }); + + describe('toggleRowSelect', () => { + it('should select unselected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.toggleRowSelect(1); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.selectedRows.has(1)).toBe(true); + }); + + it('should deselect selected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.toggleRowSelect(1); + }); + + act(() => { + result.current.toggleRowSelect(2); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.selectedRows.has(1)).toBe(false); + }); + + it('should select another unselected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.toggleRowSelect(1); + }); + + act(() => { + result.current.toggleRowSelect(2); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.selectedRows.has(1)).toBe(false); + expect(result.current.selectedRows.has(2)).toBe(true); + }); + }); + + describe('allRowsSelected', () => { + it('should return true if there is a selected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.allRowsSelected).toBe(true); + }); + + it('should return false if there is no selected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + expect(result.current.selectedRows.size).toBe(0); + expect(result.current.allRowsSelected).toBe(false); + }); + }); + + describe('someRowsSelected', () => { + it('should return true if there is a selected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + act(() => { + result.current.selectRow(1); + }); + + expect(result.current.selectedRows.size).toBe(1); + expect(result.current.someRowsSelected).toBe(true); + }); + + it('should return false if there is no selected row', () => { + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + expect(result.current.selectedRows.size).toBe(0); + expect(result.current.someRowsSelected).toBe(false); + }); + }); + }); +}); diff --git a/packages/react-components/react-table/src/hooks/useSelection.ts b/packages/react-components/react-table/src/hooks/useSelection.ts index 5a043abb585249..45e5c289625838 100644 --- a/packages/react-components/react-table/src/hooks/useSelection.ts +++ b/packages/react-components/react-table/src/hooks/useSelection.ts @@ -1,14 +1,38 @@ -import { GetRowIdInternal, SelectionStateInternal, UseTableOptions } from './types'; -import { useMultipleSelection } from './useMultipleSelection'; -import { useSingleSelection } from './useSingleSelection'; +import * as React from 'react'; +import { useEventCallback, usePrevious } from '@fluentui/react-utilities'; +import { createSelectionManager } from './selectionManager'; +import { GetRowIdInternal, RowId, SelectionMode, SelectionStateInternal } from './types'; export function useSelection( - selectionMode: UseTableOptions['selectionMode'], + selectionMode: SelectionMode, items: TItem[], getRowId: GetRowIdInternal, ): SelectionStateInternal { - const multipleSelectionState = useMultipleSelection(items, getRowId); - const singleSelectionState = useSingleSelection(); + const prevSelectionMode = usePrevious(selectionMode); + const [selected, setSelected] = React.useState(() => new Set()); + const [selectionManager, setSelectionManager] = React.useState(() => + createSelectionManager(selectionMode, setSelected), + ); - return selectionMode === 'multiselect' ? multipleSelectionState : singleSelectionState; + React.useEffect(() => { + if (prevSelectionMode !== selectionMode) { + setSelectionManager(createSelectionManager(selectionMode, setSelected)); + } + }, [selectionMode, prevSelectionMode]); + + const toggleSelectAllRows: SelectionStateInternal['toggleSelectAllRows'] = useEventCallback(() => { + selectionManager.toggleSelectAll(items.map((item, i) => getRowId(item, i))); + }); + + return { + someRowsSelected: selected.size > 0, + allRowsSelected: selectionMode === 'single' ? selected.size > 0 : selected.size === items.length, + selectedRows: selected, + toggleRowSelect: selectionManager.toggleSelect, + toggleSelectAllRows, + clearSelection: selectionManager.clearSelection, + deSelectRow: selectionManager.deSelect, + selectRow: selectionManager.select, + isRowSelected: selectionManager.isSelected, + }; } diff --git a/packages/react-components/react-table/src/hooks/useSingleSelection.test.ts b/packages/react-components/react-table/src/hooks/useSingleSelection.test.ts deleted file mode 100644 index 4d38b5c68037fd..00000000000000 --- a/packages/react-components/react-table/src/hooks/useSingleSelection.test.ts +++ /dev/null @@ -1,157 +0,0 @@ -import { renderHook, act } from '@testing-library/react-hooks'; -import { useSingleSelection } from './useSingleSelection'; - -describe('useSingleSelection', () => { - describe('toggleSelectAllRows', () => { - it('should be a noop', () => { - const { result } = renderHook(() => useSingleSelection()); - - result.current.toggleSelectAllRows(); - expect(result.current.selectedRows.size).toBe(0); - - result.current.toggleSelectAllRows(); - expect(result.current.selectedRows.size).toBe(0); - }); - }); - describe('clearSelection', () => { - it('should clear selection', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.selectRow(1); - }); - act(() => { - result.current.clearSelection(); - }); - - expect(result.current.selectedRows.size).toBe(0); - }); - }); - - describe('selectRow', () => { - it('should select row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.selectRow(1); - }); - - expect(result.current.selectedRows.has(1)).toBe(true); - }); - - it('should select another row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.selectRow(1); - }); - - act(() => { - result.current.selectRow(2); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.selectedRows.has(2)).toBe(true); - }); - }); - - describe('deSelectRow', () => { - it('should make row unselected', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.selectRow(1); - }); - - act(() => { - result.current.deSelectRow(1); - }); - - expect(result.current.selectedRows.size).toBe(0); - }); - }); - - describe('toggleRowSelect', () => { - it('should select unselected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.toggleRowSelect(1); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.selectedRows.has(1)).toBe(true); - }); - - it('should deselect selected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.toggleRowSelect(1); - }); - - act(() => { - result.current.toggleRowSelect(2); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.selectedRows.has(1)).toBe(false); - }); - - it('should select another unselected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.toggleRowSelect(1); - }); - - act(() => { - result.current.toggleRowSelect(2); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.selectedRows.has(1)).toBe(false); - expect(result.current.selectedRows.has(2)).toBe(true); - }); - }); - - describe('allRowsSelected', () => { - it('should return true if there is a selected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.selectRow(1); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.allRowsSelected).toBe(true); - }); - - it('should return false if there is no selected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - expect(result.current.selectedRows.size).toBe(0); - expect(result.current.allRowsSelected).toBe(false); - }); - }); - - describe('someRowsSelected', () => { - it('should return true if there is a selected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - act(() => { - result.current.selectRow(1); - }); - - expect(result.current.selectedRows.size).toBe(1); - expect(result.current.someRowsSelected).toBe(true); - }); - - it('should return false if there is no selected row', () => { - const { result } = renderHook(() => useSingleSelection()); - - expect(result.current.selectedRows.size).toBe(0); - expect(result.current.someRowsSelected).toBe(false); - }); - }); -}); diff --git a/packages/react-components/react-table/src/hooks/useSingleSelection.ts b/packages/react-components/react-table/src/hooks/useSingleSelection.ts deleted file mode 100644 index 515fea864d5552..00000000000000 --- a/packages/react-components/react-table/src/hooks/useSingleSelection.ts +++ /dev/null @@ -1,32 +0,0 @@ -import * as React from 'react'; -import type { SelectionStateInternal, RowId } from './types'; - -export function useSingleSelection(): SelectionStateInternal { - const [selected, setSelected] = React.useState(); - const toggleRowSelect: SelectionStateInternal['toggleRowSelect'] = React.useCallback(rowId => { - setSelected(rowId); - }, []); - - const clearSelection = React.useCallback(() => { - setSelected(undefined); - }, []); - - const isRowSelected: SelectionStateInternal['isRowSelected'] = React.useCallback( - (rowId: RowId) => { - return selected === rowId; - }, - [selected], - ); - - return { - isRowSelected, - deSelectRow: clearSelection, - clearSelection, - selectRow: toggleRowSelect, - toggleSelectAllRows: () => undefined, - toggleRowSelect, - selectedRows: new Set(selected !== undefined ? [selected] : []), - allRowsSelected: !!selected, - someRowsSelected: !!selected, - }; -} From 4fd233ed3f4e20c82fc2841613fc2e95c482fa8e Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 16 Aug 2022 12:55:59 +0200 Subject: [PATCH 2/6] changefile --- ...i-react-table-8af09ced-5374-4d8e-ab4f-6fd3d444e0e6.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-table-8af09ced-5374-4d8e-ab4f-6fd3d444e0e6.json diff --git a/change/@fluentui-react-table-8af09ced-5374-4d8e-ab4f-6fd3d444e0e6.json b/change/@fluentui-react-table-8af09ced-5374-4d8e-ab4f-6fd3d444e0e6.json new file mode 100644 index 00000000000000..8a47982700f333 --- /dev/null +++ b/change/@fluentui-react-table-8af09ced-5374-4d8e-ab4f-6fd3d444e0e6.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "refactor(useTable): selection manager to avoid calling multiple hooks", + "packageName": "@fluentui/react-table", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "patch" +} From 1efba70f507c37e37aa03b2d9d2750dc5237c82a Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 17 Aug 2022 12:10:26 +0200 Subject: [PATCH 3/6] pr suggestions --- .../react-table/etc/react-table.api.md | 6 +- .../react-table/src/hooks/selectionManager.ts | 119 +++++++++--------- .../react-table/src/hooks/types.ts | 12 +- .../src/hooks/useSelection.test.ts | 60 +++++---- .../react-table/src/hooks/useSelection.ts | 14 +-- .../react-table/src/hooks/useTable.test.ts | 6 +- .../react-table/src/hooks/useTable.ts | 18 +-- .../stories/Table/MultipleSelect.stories.tsx | 10 +- .../stories/Table/SingleSelect.stories.tsx | 6 +- 9 files changed, 125 insertions(+), 126 deletions(-) diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index f06f1a8b2f9538..5058c6463f28aa 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -70,14 +70,14 @@ export interface RowState { // @public (undocumented) export interface SelectionState { allRowsSelected: boolean; - clearSelection: () => void; + clearRows: () => void; deSelectRow: (rowId: RowId) => void; isRowSelected: (rowId: RowId) => boolean; selectedRows: RowId[]; selectRow: (rowId: RowId) => void; someRowsSelected: boolean; - toggleRowSelect: (rowId: RowId) => void; - toggleSelectAllRows: () => void; + toggleAllRows: () => void; + toggleRow: (rowId: RowId) => void; } // @public (undocumented) diff --git a/packages/react-components/react-table/src/hooks/selectionManager.ts b/packages/react-components/react-table/src/hooks/selectionManager.ts index 84774f1920010b..71004fd42ea962 100644 --- a/packages/react-components/react-table/src/hooks/selectionManager.ts +++ b/packages/react-components/react-table/src/hooks/selectionManager.ts @@ -3,12 +3,12 @@ import { SelectionMode } from './types'; type OnSelectionChangeCallback = (selectedItems: Set) => void; export interface SelectionManager { - toggleSelect(id: SelectionItemId): void; - select(id: SelectionItemId): void; - deSelect(id: SelectionItemId): void; - clearSelection(): void; + toggleItem(id: SelectionItemId): void; + selectItem(id: SelectionItemId): void; + deSelectItem(id: SelectionItemId): void; + clearItems(): void; isSelected(id: SelectionItemId): boolean; - toggleSelectAll(itemIds: SelectionItemId[]): void; + toggleAllItems(itemIds: SelectionItemId[]): void; } export type SelectionItemId = string | number; @@ -17,96 +17,97 @@ export function createSelectionManager( mode: SelectionMode, onSelectionChange: OnSelectionChangeCallback = () => undefined, ): SelectionManager { - const selectedItems = new Set(); - return mode === 'multiselect' - ? createMultipleSelectionManager(selectedItems, onSelectionChange) - : createSingleSelectionManager(selectedItems, onSelectionChange); + const managerFactory = mode === 'multiselect' ? createMultipleSelectionManager : createSingleSelectionManager; + + return managerFactory(onSelectionChange); } -function createMultipleSelectionManager( - set: Set, - onSelectionChange: OnSelectionChangeCallback, -): SelectionManager { - const toggleSelectAll = (itemIds: SelectionItemId[]) => { - if (set.size === itemIds.length) { - set.clear(); +function createMultipleSelectionManager(onSelectionChange: OnSelectionChangeCallback): SelectionManager { + const selectedItems = new Set(); + const toggleAllItems = (itemIds: SelectionItemId[]) => { + const allItemsSelected = itemIds.every(itemId => selectedItems.has(itemId)); + + if (allItemsSelected) { + selectedItems.clear(); } else { - itemIds.forEach(itemId => set.add(itemId)); + itemIds.forEach(itemId => selectedItems.add(itemId)); } - onSelectionChange(new Set(set)); + onSelectionChange(new Set(selectedItems)); }; - const toggleSelect = (itemId: SelectionItemId) => { - if (set.has(itemId)) { - set.delete(itemId); + const toggleItem = (itemId: SelectionItemId) => { + if (selectedItems.has(itemId)) { + selectedItems.delete(itemId); } else { - set.add(itemId); + selectedItems.add(itemId); } - onSelectionChange(new Set(set)); + onSelectionChange(new Set(selectedItems)); }; - const select = (itemId: SelectionItemId) => { - set.add(itemId); - onSelectionChange(new Set(set)); + const selectItem = (itemId: SelectionItemId) => { + selectedItems.add(itemId); + onSelectionChange(new Set(selectedItems)); }; - const deSelect = (itemId: SelectionItemId) => { - set.delete(itemId); - onSelectionChange(new Set(set)); + const deSelectItem = (itemId: SelectionItemId) => { + selectedItems.delete(itemId); + onSelectionChange(new Set(selectedItems)); }; - const clearSelection = () => { - set.clear(); - onSelectionChange(new Set(set)); + const clearItems = () => { + selectedItems.clear(); + onSelectionChange(new Set(selectedItems)); }; const isSelected = (itemId: SelectionItemId) => { - return set.has(itemId); + return selectedItems.has(itemId); }; return { - toggleSelect, - select, - deSelect, - clearSelection, + toggleItem, + selectItem, + deSelectItem, + clearItems, isSelected, - toggleSelectAll, + toggleAllItems, }; } -function createSingleSelectionManager( - set: Set, - onSelectionChange: OnSelectionChangeCallback, -): SelectionManager { - const toggleSelect = (itemId: SelectionItemId) => { - set.clear(); - set.add(itemId); - onSelectionChange(new Set(set)); +function createSingleSelectionManager(onSelectionChange: OnSelectionChangeCallback): SelectionManager { + let selectedItem: SelectionItemId | undefined = undefined; + const toggleItem = (itemId: SelectionItemId) => { + selectedItem = itemId; + onSelectionChange(new Set([selectedItem])); }; - const clearSelection = () => { - set.clear(); - onSelectionChange(new Set(set)); + const clearItems = () => { + selectedItem = undefined; + onSelectionChange(new Set()); }; const isSelected = (itemId: SelectionItemId) => { - return set.has(itemId); + return selectedItem === itemId; }; - const select = (itemId: SelectionItemId) => { - set.clear(); - set.add(itemId); - onSelectionChange(new Set(set)); + const selectItem = (itemId: SelectionItemId) => { + selectedItem = itemId; + onSelectionChange(new Set([selectedItem])); }; return { - deSelect: clearSelection, - select, - toggleSelectAll: () => undefined, - toggleSelect, - clearSelection, + deSelectItem: clearItems, + selectItem, + toggleAllItems: () => { + if (process.env.NODE_ENV !== 'production') { + throw new Error('[react-table]: `toggleAllItems` should not be used in single selection mode'); + } + + return undefined; + }, + toggleItem, + clearItems, isSelected, }; } diff --git a/packages/react-components/react-table/src/hooks/types.ts b/packages/react-components/react-table/src/hooks/types.ts index c677c2c23008b5..973933eb3fc0a9 100644 --- a/packages/react-components/react-table/src/hooks/types.ts +++ b/packages/react-components/react-table/src/hooks/types.ts @@ -36,11 +36,11 @@ export interface UseTableOptions = RowS } export interface SelectionStateInternal { - clearSelection: () => void; + clearRows: () => void; deSelectRow: (rowId: RowId) => void; selectRow: (rowId: RowId) => void; - toggleSelectAllRows: () => void; - toggleRowSelect: (rowId: RowId) => void; + toggleAllRows: () => void; + toggleRow: (rowId: RowId) => void; isRowSelected: (rowId: RowId) => boolean; selectedRows: Set; allRowsSelected: boolean; @@ -75,7 +75,7 @@ export interface SelectionState { /** * Clears all selected rows */ - clearSelection: () => void; + clearRows: () => void; /** * Selects single row */ @@ -87,11 +87,11 @@ export interface SelectionState { /** * Toggle selection of all rows */ - toggleSelectAllRows: () => void; + toggleAllRows: () => void; /** * Toggle selection of single row */ - toggleRowSelect: (rowId: RowId) => void; + toggleRow: (rowId: RowId) => void; /** * Collection of row ids corresponding to selected rows */ diff --git a/packages/react-components/react-table/src/hooks/useSelection.test.ts b/packages/react-components/react-table/src/hooks/useSelection.test.ts index 6c88d430e398c9..a60066b42a6b69 100644 --- a/packages/react-components/react-table/src/hooks/useSelection.test.ts +++ b/packages/react-components/react-table/src/hooks/useSelection.test.ts @@ -10,18 +10,18 @@ describe('useSelection', () => { it('should use custom row id', () => { const { result } = renderHook(() => useSelection('multiselect', items, (item: { value: string }) => item.value)); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); expect(Array.from(result.current.selectedRows)).toEqual(items.map(item => item.value)); }); - describe('toggleSelectAllRows', () => { + describe('toggleAllRows', () => { it('should select all rows', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); expect(result.current.selectedRows.size).toBe(items.length); expect(Array.from(result.current.selectedRows)).toEqual(items.map((_, i) => i)); @@ -31,25 +31,25 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); expect(result.current.selectedRows.size).toBe(0); }); }); - describe('clearSelection', () => { + describe('clearRows', () => { it('should clear selection', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); act(() => { - result.current.clearSelection(); + result.current.clearRows(); }); expect(result.current.selectedRows.size).toBe(0); @@ -100,12 +100,12 @@ describe('useSelection', () => { }); }); - describe('toggleRowSelect', () => { + describe('toggleRow', () => { it('should select unselected row', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); expect(result.current.selectedRows.size).toBe(1); @@ -116,11 +116,11 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); expect(result.current.selectedRows.size).toBe(0); @@ -131,11 +131,11 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); act(() => { - result.current.toggleRowSelect(2); + result.current.toggleRow(2); }); expect(result.current.selectedRows.size).toBe(2); @@ -149,7 +149,7 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); expect(result.current.allRowsSelected).toBe(true); @@ -166,7 +166,7 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { - result.current.toggleSelectAllRows(); + result.current.toggleAllRows(); }); act(() => { @@ -200,18 +200,16 @@ describe('useSelection', () => { }); describe('single select', () => { - describe('toggleSelectAllRows', () => { - it('should be a noop', () => { + describe('toggleAllRows', () => { + it('should throw', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); - result.current.toggleSelectAllRows(); - expect(result.current.selectedRows.size).toBe(0); - - result.current.toggleSelectAllRows(); - expect(result.current.selectedRows.size).toBe(0); + expect(result.current.toggleAllRows).toThrowErrorMatchingInlineSnapshot( + `"[react-table]: \`toggleAllItems\` should not be used in single selection mode"`, + ); }); }); - describe('clearSelection', () => { + describe('clearRows', () => { it('should clear selection', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); @@ -219,7 +217,7 @@ describe('useSelection', () => { result.current.selectRow(1); }); act(() => { - result.current.clearSelection(); + result.current.clearRows(); }); expect(result.current.selectedRows.size).toBe(0); @@ -269,12 +267,12 @@ describe('useSelection', () => { }); }); - describe('toggleRowSelect', () => { + describe('toggleRow', () => { it('should select unselected row', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); expect(result.current.selectedRows.size).toBe(1); @@ -285,11 +283,11 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); act(() => { - result.current.toggleRowSelect(2); + result.current.toggleRow(2); }); expect(result.current.selectedRows.size).toBe(1); @@ -300,11 +298,11 @@ describe('useSelection', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); act(() => { - result.current.toggleRowSelect(1); + result.current.toggleRow(1); }); act(() => { - result.current.toggleRowSelect(2); + result.current.toggleRow(2); }); expect(result.current.selectedRows.size).toBe(1); diff --git a/packages/react-components/react-table/src/hooks/useSelection.ts b/packages/react-components/react-table/src/hooks/useSelection.ts index 45e5c289625838..cc6133bd06249a 100644 --- a/packages/react-components/react-table/src/hooks/useSelection.ts +++ b/packages/react-components/react-table/src/hooks/useSelection.ts @@ -20,19 +20,19 @@ export function useSelection( } }, [selectionMode, prevSelectionMode]); - const toggleSelectAllRows: SelectionStateInternal['toggleSelectAllRows'] = useEventCallback(() => { - selectionManager.toggleSelectAll(items.map((item, i) => getRowId(item, i))); + const toggleAllRows: SelectionStateInternal['toggleAllRows'] = useEventCallback(() => { + selectionManager.toggleAllItems(items.map((item, i) => getRowId(item, i))); }); return { someRowsSelected: selected.size > 0, allRowsSelected: selectionMode === 'single' ? selected.size > 0 : selected.size === items.length, selectedRows: selected, - toggleRowSelect: selectionManager.toggleSelect, - toggleSelectAllRows, - clearSelection: selectionManager.clearSelection, - deSelectRow: selectionManager.deSelect, - selectRow: selectionManager.select, + toggleRow: selectionManager.toggleItem, + toggleAllRows, + clearRows: selectionManager.clearItems, + deSelectRow: selectionManager.deSelectItem, + selectRow: selectionManager.selectItem, isRowSelected: selectionManager.isSelected, }; } diff --git a/packages/react-components/react-table/src/hooks/useTable.test.ts b/packages/react-components/react-table/src/hooks/useTable.test.ts index 1bd00737203fac..f679da54c6af22 100644 --- a/packages/react-components/react-table/src/hooks/useTable.test.ts +++ b/packages/react-components/react-table/src/hooks/useTable.test.ts @@ -52,14 +52,14 @@ describe('useTable', () => { expect(result.current.selection).toMatchInlineSnapshot(` Object { "allRowsSelected": false, - "clearSelection": [Function], + "clearRows": [Function], "deSelectRow": [Function], "isRowSelected": [Function], "selectRow": [Function], "selectedRows": Array [], "someRowsSelected": false, - "toggleRowSelect": [Function], - "toggleSelectAllRows": [Function], + "toggleAllRows": [Function], + "toggleRow": [Function], } `); }); diff --git a/packages/react-components/react-table/src/hooks/useTable.ts b/packages/react-components/react-table/src/hooks/useTable.ts index c0939c939483e7..8f5088026e5342 100644 --- a/packages/react-components/react-table/src/hooks/useTable.ts +++ b/packages/react-components/react-table/src/hooks/useTable.ts @@ -29,12 +29,12 @@ export function useTable = RowState = RowState ({ isRowSelected, - clearSelection, + clearRows, deSelectRow, selectRow, - toggleSelectAllRows, - toggleRowSelect, + toggleAllRows, + toggleRow, selectedRows: Array.from(selectedRows), allRowsSelected, someRowsSelected, }), [ isRowSelected, - clearSelection, + clearRows, deSelectRow, selectRow, - toggleSelectAllRows, - toggleRowSelect, + toggleAllRows, + toggleRow, selectedRows, allRowsSelected, someRowsSelected, diff --git a/packages/react-components/react-table/src/stories/Table/MultipleSelect.stories.tsx b/packages/react-components/react-table/src/stories/Table/MultipleSelect.stories.tsx index 0c8a9748415744..5ebd000c26c1a5 100644 --- a/packages/react-components/react-table/src/stories/Table/MultipleSelect.stories.tsx +++ b/packages/react-components/react-table/src/stories/Table/MultipleSelect.stories.tsx @@ -96,13 +96,13 @@ const columns: ColumnDefinition[] = [ export const MultipleSelect = () => { const { rows, - selection: { allRowsSelected, someRowsSelected, toggleSelectAllRows }, + selection: { allRowsSelected, someRowsSelected, toggleAllRows }, } = useTable({ columns, items, rowEnhancer: (row, { selection }) => ({ ...row, - toggleSelect: () => selection.toggleRowSelect(row.rowId), + toggle: () => selection.toggleRow(row.rowId), selected: selection.isRowSelected(row.rowId), }), }); @@ -113,7 +113,7 @@ export const MultipleSelect = () => { File Author @@ -122,8 +122,8 @@ export const MultipleSelect = () => { - {rows.map(({ item, selected, toggleSelect }) => ( - + {rows.map(({ item, selected, toggle }) => ( + {item.file.label} }>{item.author.label} diff --git a/packages/react-components/react-table/src/stories/Table/SingleSelect.stories.tsx b/packages/react-components/react-table/src/stories/Table/SingleSelect.stories.tsx index b7eeba6519ad21..fbd52a290f613a 100644 --- a/packages/react-components/react-table/src/stories/Table/SingleSelect.stories.tsx +++ b/packages/react-components/react-table/src/stories/Table/SingleSelect.stories.tsx @@ -101,7 +101,7 @@ export const SingleSelect = () => { rowEnhancer: (row, { selection }) => ({ ...row, selected: selection.isRowSelected(row.rowId), - toggleSelect: () => selection.toggleRowSelect(row.rowId), + toggle: () => selection.toggleRow(row.rowId), }), }); @@ -117,8 +117,8 @@ export const SingleSelect = () => { - {rows.map(({ item, toggleSelect, selected }) => ( - + {rows.map(({ item, toggle, selected }) => ( + {item.file.label} }>{item.author.label} From 604ef690c0dd89f42aac9f27c44c8be02d0644dc Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 17 Aug 2022 12:15:03 +0200 Subject: [PATCH 4/6] add node env test --- .../react-table/src/hooks/useSelection.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/react-components/react-table/src/hooks/useSelection.test.ts b/packages/react-components/react-table/src/hooks/useSelection.test.ts index a60066b42a6b69..9cc2a95dc10e60 100644 --- a/packages/react-components/react-table/src/hooks/useSelection.test.ts +++ b/packages/react-components/react-table/src/hooks/useSelection.test.ts @@ -201,13 +201,23 @@ describe('useSelection', () => { describe('single select', () => { describe('toggleAllRows', () => { - it('should throw', () => { + it('should throw when not in production', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); expect(result.current.toggleAllRows).toThrowErrorMatchingInlineSnapshot( `"[react-table]: \`toggleAllItems\` should not be used in single selection mode"`, ); }); + + it('should be a noop in production', () => { + const nodeEnv = (process.env.NODE_ENV = 'production'); + const { result } = renderHook(() => useSelection('single', items, getRowId)); + + result.current.toggleAllRows; + expect(result.current.selectedRows.size).toBe(0); + + process.env.NODE_ENV = nodeEnv; + }); }); describe('clearRows', () => { it('should clear selection', () => { From d63d3f0661f37e1543d2dccc06c3675bb968c41f Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 17 Aug 2022 12:37:18 +0200 Subject: [PATCH 5/6] deSelect to deselect --- .../react-table/src/hooks/selectionManager.ts | 8 ++++---- .../react-components/react-table/src/hooks/types.ts | 4 ++-- .../react-table/src/hooks/useSelection.test.ts | 12 ++++++------ .../react-table/src/hooks/useSelection.ts | 2 +- .../react-table/src/hooks/useTable.test.ts | 2 +- .../react-table/src/hooks/useTable.ts | 6 +++--- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/react-components/react-table/src/hooks/selectionManager.ts b/packages/react-components/react-table/src/hooks/selectionManager.ts index 71004fd42ea962..87fa5a9b2c40ca 100644 --- a/packages/react-components/react-table/src/hooks/selectionManager.ts +++ b/packages/react-components/react-table/src/hooks/selectionManager.ts @@ -5,7 +5,7 @@ type OnSelectionChangeCallback = (selectedItems: Set) => void; export interface SelectionManager { toggleItem(id: SelectionItemId): void; selectItem(id: SelectionItemId): void; - deSelectItem(id: SelectionItemId): void; + deselectItem(id: SelectionItemId): void; clearItems(): void; isSelected(id: SelectionItemId): boolean; toggleAllItems(itemIds: SelectionItemId[]): void; @@ -51,7 +51,7 @@ function createMultipleSelectionManager(onSelectionChange: OnSelectionChangeCall onSelectionChange(new Set(selectedItems)); }; - const deSelectItem = (itemId: SelectionItemId) => { + const deselectItem = (itemId: SelectionItemId) => { selectedItems.delete(itemId); onSelectionChange(new Set(selectedItems)); }; @@ -68,7 +68,7 @@ function createMultipleSelectionManager(onSelectionChange: OnSelectionChangeCall return { toggleItem, selectItem, - deSelectItem, + deselectItem, clearItems, isSelected, toggleAllItems, @@ -97,7 +97,7 @@ function createSingleSelectionManager(onSelectionChange: OnSelectionChangeCallba }; return { - deSelectItem: clearItems, + deselectItem: clearItems, selectItem, toggleAllItems: () => { if (process.env.NODE_ENV !== 'production') { diff --git a/packages/react-components/react-table/src/hooks/types.ts b/packages/react-components/react-table/src/hooks/types.ts index 973933eb3fc0a9..324ec6950eed79 100644 --- a/packages/react-components/react-table/src/hooks/types.ts +++ b/packages/react-components/react-table/src/hooks/types.ts @@ -37,7 +37,7 @@ export interface UseTableOptions = RowS export interface SelectionStateInternal { clearRows: () => void; - deSelectRow: (rowId: RowId) => void; + deselectRow: (rowId: RowId) => void; selectRow: (rowId: RowId) => void; toggleAllRows: () => void; toggleRow: (rowId: RowId) => void; @@ -83,7 +83,7 @@ export interface SelectionState { /** * De-selects single row */ - deSelectRow: (rowId: RowId) => void; + deselectRow: (rowId: RowId) => void; /** * Toggle selection of all rows */ diff --git a/packages/react-components/react-table/src/hooks/useSelection.test.ts b/packages/react-components/react-table/src/hooks/useSelection.test.ts index 9cc2a95dc10e60..768a8d318b6ff0 100644 --- a/packages/react-components/react-table/src/hooks/useSelection.test.ts +++ b/packages/react-components/react-table/src/hooks/useSelection.test.ts @@ -27,7 +27,7 @@ describe('useSelection', () => { expect(Array.from(result.current.selectedRows)).toEqual(items.map((_, i) => i)); }); - it('should deSelect all rows', () => { + it('should deselect all rows', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); act(() => { @@ -84,7 +84,7 @@ describe('useSelection', () => { }); }); - describe('deSelectRow', () => { + describe('deselectRow', () => { it('should make row unselected', () => { const { result } = renderHook(() => useSelection('multiselect', items, getRowId)); @@ -93,7 +93,7 @@ describe('useSelection', () => { }); act(() => { - result.current.deSelectRow(1); + result.current.deselectRow(1); }); expect(result.current.selectedRows.size).toBe(0); @@ -170,7 +170,7 @@ describe('useSelection', () => { }); act(() => { - result.current.deSelectRow(1); + result.current.deselectRow(1); }); expect(result.current.selectedRows.size).toBe(3); @@ -261,7 +261,7 @@ describe('useSelection', () => { }); }); - describe('deSelectRow', () => { + describe('deselectRow', () => { it('should make row unselected', () => { const { result } = renderHook(() => useSelection('single', items, getRowId)); @@ -270,7 +270,7 @@ describe('useSelection', () => { }); act(() => { - result.current.deSelectRow(1); + result.current.deselectRow(1); }); expect(result.current.selectedRows.size).toBe(0); diff --git a/packages/react-components/react-table/src/hooks/useSelection.ts b/packages/react-components/react-table/src/hooks/useSelection.ts index cc6133bd06249a..cff6a47e6d2fc3 100644 --- a/packages/react-components/react-table/src/hooks/useSelection.ts +++ b/packages/react-components/react-table/src/hooks/useSelection.ts @@ -31,7 +31,7 @@ export function useSelection( toggleRow: selectionManager.toggleItem, toggleAllRows, clearRows: selectionManager.clearItems, - deSelectRow: selectionManager.deSelectItem, + deselectRow: selectionManager.deselectItem, selectRow: selectionManager.selectItem, isRowSelected: selectionManager.isSelected, }; diff --git a/packages/react-components/react-table/src/hooks/useTable.test.ts b/packages/react-components/react-table/src/hooks/useTable.test.ts index f679da54c6af22..b0f46d7bbee64e 100644 --- a/packages/react-components/react-table/src/hooks/useTable.test.ts +++ b/packages/react-components/react-table/src/hooks/useTable.test.ts @@ -53,7 +53,7 @@ describe('useTable', () => { Object { "allRowsSelected": false, "clearRows": [Function], - "deSelectRow": [Function], + "deselectRow": [Function], "isRowSelected": [Function], "selectRow": [Function], "selectedRows": Array [], diff --git a/packages/react-components/react-table/src/hooks/useTable.ts b/packages/react-components/react-table/src/hooks/useTable.ts index 8f5088026e5342..e9ddf646181129 100644 --- a/packages/react-components/react-table/src/hooks/useTable.ts +++ b/packages/react-components/react-table/src/hooks/useTable.ts @@ -36,14 +36,14 @@ export function useTable = RowState ({ isRowSelected, clearRows, - deSelectRow, + deselectRow, selectRow, toggleAllRows, toggleRow, @@ -54,7 +54,7 @@ export function useTable = RowState Date: Wed, 17 Aug 2022 13:44:32 +0200 Subject: [PATCH 6/6] update md --- packages/react-components/react-table/etc/react-table.api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index 5058c6463f28aa..f80ec68a76cd58 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -71,7 +71,7 @@ export interface RowState { export interface SelectionState { allRowsSelected: boolean; clearRows: () => void; - deSelectRow: (rowId: RowId) => void; + deselectRow: (rowId: RowId) => void; isRowSelected: (rowId: RowId) => boolean; selectedRows: RowId[]; selectRow: (rowId: RowId) => void;