From b4aa285376c3eca935d99955e336441c351933f4 Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 09:57:54 -0400 Subject: [PATCH 1/7] Prevent row selection when IndexTable is not selectable --- src/components/IndexTable/components/Row/Row.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/components/IndexTable/components/Row/Row.tsx b/src/components/IndexTable/components/Row/Row.tsx index b68f4befb31..9f74f70f515 100644 --- a/src/components/IndexTable/components/Row/Row.tsx +++ b/src/components/IndexTable/components/Row/Row.tsx @@ -2,6 +2,7 @@ import React, {useMemo, memo, useRef, useCallback} from 'react'; import {useToggle} from '../../../../utilities/use-toggle'; import { + useIndexValue, useIndexRow, SelectionType, useIndexSelectionChange, @@ -32,6 +33,7 @@ export const Row = memo(function Row({ status, onNavigation, }: RowProps) { + const {selectable} = useIndexValue(); const {selectMode, condensed} = useIndexRow(); const onSelectionChange = useIndexSelectionChange(); const { @@ -57,9 +59,9 @@ export const Row = memo(function Row({ ? SelectionType.Multi : SelectionType.Single; - onSelectionChange(selectionType, !selected, id, position); + if (selectable) onSelectionChange(selectionType, !selected, id, position); }, - [id, onSelectionChange, position, selected], + [id, selectable, onSelectionChange, position, selected], ); const contextValue = useMemo( From 44a1667e27111f104a68c9a100e257048c5b64aa Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 10:13:11 -0400 Subject: [PATCH 2/7] No checkboxes when an IndexTable is not selectable --- src/components/IndexTable/IndexTable.tsx | 4 ++-- src/components/IndexTable/components/Row/Row.tsx | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/components/IndexTable/IndexTable.tsx b/src/components/IndexTable/IndexTable.tsx index 7f7565d592c..f24fb09c605 100644 --- a/src/components/IndexTable/IndexTable.tsx +++ b/src/components/IndexTable/IndexTable.tsx @@ -568,7 +568,7 @@ function IndexTableBase({ const isSecond = index === 0; const headingContentClassName = classNames( styles.TableHeading, - isSecond && styles['TableHeading-second'], + selectable && isSecond && styles['TableHeading-second'], ); const stickyPositioningStyle = @@ -589,7 +589,7 @@ function IndexTableBase({ ); - if (index !== 0) return headingContent; + if (index !== 0 || !selectable) return headingContent; const checkboxClassName = classNames( styles.TableHeading, diff --git a/src/components/IndexTable/components/Row/Row.tsx b/src/components/IndexTable/components/Row/Row.tsx index 9f74f70f515..7eda656eac8 100644 --- a/src/components/IndexTable/components/Row/Row.tsx +++ b/src/components/IndexTable/components/Row/Row.tsx @@ -115,6 +115,7 @@ export const Row = memo(function Row({ }; const RowWrapper = condensed ? 'li' : 'tr'; + const checkbox = selectable ? : null; return ( @@ -127,7 +128,7 @@ export const Row = memo(function Row({ onClick={handleRowClick} ref={tableRowRef} > - + {checkbox} {children} From c753f5527e67a3aae1665812d3c5dc62838bd7c7 Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 10:28:29 -0400 Subject: [PATCH 3/7] Differentiate use of the term "selectable" Within the IndexProvider the value is accepted as a prop but isn't being used, when it should be. There's already an existing identifier "selectable" which can be renamed as "actionable" because it indicates whether or not actions can be made (are there bulk actions available). Similarly in the useBulkSelectionData hook there's an existing identifier "selectable" where it means "does it currently have any selections. It's a temporary variable created to indicate whether or not any selections have been made, so we rename it to "hasSelections" even though the scope doesn't conflict - just to make it clearer. --- src/components/IndexProvider/IndexProvider.tsx | 2 +- src/components/IndexTable/IndexTable.tsx | 5 +++-- src/utilities/index-provider/hooks.ts | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/components/IndexProvider/IndexProvider.tsx b/src/components/IndexProvider/IndexProvider.tsx index 9a087db1130..1d7284000a4 100644 --- a/src/components/IndexProvider/IndexProvider.tsx +++ b/src/components/IndexProvider/IndexProvider.tsx @@ -18,6 +18,7 @@ export function IndexProvider({ itemCount, hasMoreItems, condensed, + selectable, }: IndexProviderProps) { const { paginatedSelectAllText, @@ -26,7 +27,6 @@ export function IndexProvider({ resourceName, selectMode, bulkSelectState, - selectable, } = useBulkSelectionData({ selectedItemsCount, itemCount, diff --git a/src/components/IndexTable/IndexTable.tsx b/src/components/IndexTable/IndexTable.tsx index f24fb09c605..cee04d08da8 100644 --- a/src/components/IndexTable/IndexTable.tsx +++ b/src/components/IndexTable/IndexTable.tsx @@ -70,6 +70,7 @@ function IndexTableBase({ bulkSelectState, resourceName, bulkActionsAccessibilityLabel, + selectable, selectMode, paginatedSelectAllText, itemCount, @@ -290,7 +291,7 @@ function IndexTableBase({ } }, [condensed, isSmallScreenSelectable]); - const selectable = Boolean( + const actionable = Boolean( (promotedBulkActions && promotedBulkActions.length > 0) || (bulkActions && bulkActions.length > 0), ); @@ -674,7 +675,7 @@ function IndexTableBase({ } function getPaginatedSelectAllAction() { - if (!selectable || !hasMoreItems) { + if (!actionable || !hasMoreItems) { return; } diff --git a/src/utilities/index-provider/hooks.ts b/src/utilities/index-provider/hooks.ts index e0fdba92a7d..3e94e839c42 100644 --- a/src/utilities/index-provider/hooks.ts +++ b/src/utilities/index-provider/hooks.ts @@ -48,7 +48,7 @@ export function useBulkSelectionData({ }: BulkSelectionDataOptions) { const i18n = useI18n(); - const selectable = Boolean(selectedItemsCount); + const hasSelections = Boolean(selectedItemsCount); const selectMode = selectedItemsCount === 'All' || selectedItemsCount > 0; const defaultResourceName = { @@ -82,11 +82,11 @@ export function useBulkSelectionData({ resourceName, selectMode, bulkSelectState, - selectable, + hasSelections, }; function getPaginatedSelectAllText() { - if (!selectable || !hasMoreItems) { + if (!hasSelections || !hasMoreItems) { return; } From 7a4db6cac81106b85c21b5fe8481148b15ea7223 Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 10:44:55 -0400 Subject: [PATCH 4/7] Maintain backwards compatibility of component with default selectable This was previously always the case, and when the selectable prop is not provided we would still expect it to default to true, because the main behaviour of this component is for use as a selectable index. This provides backwards compatibility, while still supporting explicitly passing this value as true/false. --- src/components/IndexTable/IndexTable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/IndexTable/IndexTable.tsx b/src/components/IndexTable/IndexTable.tsx index cee04d08da8..efb27621b8b 100644 --- a/src/components/IndexTable/IndexTable.tsx +++ b/src/components/IndexTable/IndexTable.tsx @@ -711,7 +711,7 @@ export interface IndexTableProps export function IndexTable({ children, - selectable, + selectable = true, itemCount, selectedItemsCount, resourceName: passedResourceName, From dbd5c6fc3109f8f12401305109685aeba0c67ff1 Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 10:58:55 -0400 Subject: [PATCH 5/7] Test that checkboxes do not appear in the heading when not selectable --- .../IndexTable/tests/IndexTable.test.tsx | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/components/IndexTable/tests/IndexTable.test.tsx b/src/components/IndexTable/tests/IndexTable.test.tsx index 62a8147725e..a8c871d07e2 100644 --- a/src/components/IndexTable/tests/IndexTable.test.tsx +++ b/src/components/IndexTable/tests/IndexTable.test.tsx @@ -466,4 +466,21 @@ describe('', () => { }); }); }); + + describe('not selectable', () => { + it('does not render a checkbox in the headings', () => { + const index = mountWithApp( + + {mockTableItems.map(mockRenderRow)} + , + ); + const headings = index.find('thead'); + + expect(headings).not.toContainReactComponent(Checkbox); + }); + }); }); From 94196518f1cd7ecb98501d808bd04d3408421b28 Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 11:09:38 -0400 Subject: [PATCH 6/7] Test that row does not contain a checkbox when not selectable --- .../IndexTable/components/Row/tests/Row.test.tsx | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/components/IndexTable/components/Row/tests/Row.test.tsx b/src/components/IndexTable/components/Row/tests/Row.test.tsx index 84f63932a28..b54340192b7 100644 --- a/src/components/IndexTable/components/Row/tests/Row.test.tsx +++ b/src/components/IndexTable/components/Row/tests/Row.test.tsx @@ -2,6 +2,7 @@ import React, {ReactElement} from 'react'; import {mountWithApp} from 'test-utilities'; import type {DeepPartial, ThenType} from '@shopify/useful-types'; +import {Checkbox} from '../../Checkbox'; import {IndexTable, IndexTableProps} from '../../../IndexTable'; import {RowHoveredContext} from '../../../../../utilities/index-table'; import {Row} from '../Row'; @@ -242,6 +243,17 @@ describe('', () => { className: 'TableRow statusSubdued', }); }); + + it('does not render a checkbox when not selectable', () => { + const row = mountWithTable( + + + , + {indexTableProps: {selectable: false}}, + ); + + expect(row).not.toContainReactComponent(Checkbox); + }); }); function triggerOnClick( From 52314263792b7199d609765dcf4b7af0427d870d Mon Sep 17 00:00:00 2001 From: Ryan Wilson-Perkin Date: Thu, 22 Jul 2021 11:19:47 -0400 Subject: [PATCH 7/7] Separate out rendering of headings and of checkboxes Simplifies the closure function because it no longer has multiple return signatures (returning either JSX or JSX[], depending on whether or not its the first element). Splits out a second one that's specifically for rendering the checkbox, and then explicitly calling that and pushing it into the array when selectable. --- src/components/IndexTable/IndexTable.tsx | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/components/IndexTable/IndexTable.tsx b/src/components/IndexTable/IndexTable.tsx index efb27621b8b..797af8a512a 100644 --- a/src/components/IndexTable/IndexTable.tsx +++ b/src/components/IndexTable/IndexTable.tsx @@ -296,9 +296,8 @@ function IndexTableBase({ (bulkActions && bulkActions.length > 0), ); - const headingsMarkup = headings - .map(renderHeading) - .reduce((acc, heading) => acc.concat(heading), []); + const headingsMarkup = headings.map(renderHeading); + if (selectable) headingsMarkup.unshift(renderCheckboxCell()); const bulkActionsSelectable = Boolean( promotedBulkActions.length > 0 || bulkActions.length > 0, @@ -569,7 +568,7 @@ function IndexTableBase({ const isSecond = index === 0; const headingContentClassName = classNames( styles.TableHeading, - selectable && isSecond && styles['TableHeading-second'], + isSecond && styles['TableHeading-second'], ); const stickyPositioningStyle = @@ -579,7 +578,7 @@ function IndexTableBase({ ? {left: tableHeadingRects.current[0].offsetWidth} : undefined; - const headingContent = ( + return ( ); + } - if (index !== 0 || !selectable) return headingContent; - + function renderCheckboxCell() { const checkboxClassName = classNames( styles.TableHeading, - index === 0 && styles['TableHeading-first'], + styles['TableHeading-first'], ); - const checkboxContent = ( + return ( {renderCheckboxContent()} ); - - return [checkboxContent, headingContent]; } function renderCheckboxContent() {