From c8fce9f9fcb06ccc5da54e525141dc41fd2b8e3d Mon Sep 17 00:00:00 2001 From: Jason Addleman Date: Fri, 10 Nov 2023 07:25:11 -0500 Subject: [PATCH 1/3] Improve the type safety and correctness of ChoiceList --- .changeset/old-guests-cover.md | 5 ++ .../ChoiceList/ChoiceList.stories.tsx | 68 ++++++++----------- .../src/components/ChoiceList/ChoiceList.tsx | 30 ++++---- 3 files changed, 45 insertions(+), 58 deletions(-) create mode 100644 .changeset/old-guests-cover.md diff --git a/.changeset/old-guests-cover.md b/.changeset/old-guests-cover.md new file mode 100644 index 00000000000..b29ed87597d --- /dev/null +++ b/.changeset/old-guests-cover.md @@ -0,0 +1,5 @@ +--- +'@shopify/polaris': patch +--- + +Improve the type safety and accuracy of ChoiceList diff --git a/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx b/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx index e3c6424478b..a4270268b61 100644 --- a/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx +++ b/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx @@ -6,10 +6,12 @@ export default { component: ChoiceList, } as ComponentMeta; -export function Default() { - const [selected, setSelected] = useState(['hidden']); +type HiddenOptionalRequired = 'hidden' | 'optional' | 'required'; - const handleChange = useCallback((value) => setSelected(value), []); +export function Default() { + const [selected, setSelected] = useState([ + 'hidden', + ]); return ( ); } export function WithError() { - const [selected, setSelected] = useState(['hidden']); - - const handleChange = useCallback((value) => setSelected(value), []); + const [selected, setSelected] = useState([ + 'hidden' as HiddenOptionalRequired, + ]); return ( ); } export function Magic() { - const [selected, setSelected] = useState(['hidden']); - - const handleChange = useCallback((value) => setSelected(value), []); + const [selected, setSelected] = useState([ + 'hidden', + ]); return ( ); } export function WithMultiChoice() { - const [selected, setSelected] = useState(['hidden']); - - const handleChange = useCallback((value) => setSelected(value), []); + const [selected, setSelected] = useState([] as string[]); return ( ); } export function MagicWithMultiChoice() { - const [selected, setSelected] = useState(['hidden']); - - const handleChange = useCallback((value) => setSelected(value), []); + const [selected, setSelected] = useState([]); return ( ); } export function WithChildrenContent() { - const [selected, setSelected] = useState(['none']); + const [selected, setSelected] = useState([ + 'none' as 'none' | 'minimum_purchase' | 'minimum_quantity', + ]); const [textFieldValue, setTextFieldValue] = useState(''); - const handleChoiceListChange = useCallback((value) => setSelected(value), []); - - const handleTextFieldChange = useCallback( - (value) => setTextFieldValue(value), - [], - ); - const renderChildren = useCallback( () => ( ), - [handleTextFieldChange, textFieldValue], + [textFieldValue], ); return ( @@ -164,7 +157,7 @@ export function WithChildrenContent() { }, ]} selected={selected} - onChange={handleChoiceListChange} + onChange={setSelected} /> ); } @@ -173,25 +166,18 @@ export function WithDynamicChildrenContent() { const [selected, setSelected] = useState(['none']); const [textFieldValue, setTextFieldValue] = useState(''); - const handleChoiceListChange = useCallback((value) => setSelected(value), []); - - const handleTextFieldChange = useCallback( - (value) => setTextFieldValue(value), - [], - ); - const renderChildren = useCallback( (isSelected) => isSelected && ( ), - [handleTextFieldChange, textFieldValue], + [textFieldValue], ); return ( @@ -212,7 +198,7 @@ export function WithDynamicChildrenContent() { }, ]} selected={selected} - onChange={handleChoiceListChange} + onChange={setSelected} /> ); diff --git a/polaris-react/src/components/ChoiceList/ChoiceList.tsx b/polaris-react/src/components/ChoiceList/ChoiceList.tsx index 6226bdbdec0..2db4f5a704e 100644 --- a/polaris-react/src/components/ChoiceList/ChoiceList.tsx +++ b/polaris-react/src/components/ChoiceList/ChoiceList.tsx @@ -10,9 +10,9 @@ import {Bleed} from '../Bleed'; import styles from './ChoiceList.scss'; -interface Choice { +interface Choice { /** Value of the choice */ - value: string; + value: TValue; /** Label for the choice */ label: React.ReactNode; /** A unique identifier for the choice */ @@ -27,13 +27,13 @@ interface Choice { renderChildren?(isSelected: boolean): React.ReactNode | false; } -export interface ChoiceListProps { +export interface ChoiceListProps { /** Label for list of choices */ title: React.ReactNode; /** Collection of choices */ - choices: Choice[]; + choices: Choice[] | readonly Choice[]; /** Collection of selected choices */ - selected: string[]; + selected: TValue[]; /** Name for form input */ name?: string; /** Allow merchants to select multiple options at once */ @@ -45,12 +45,12 @@ export interface ChoiceListProps { /** Disable all choices **/ disabled?: boolean; /** Callback when the selected choices change */ - onChange?(selected: string[], name: string): void; + onChange?(selected: TValue[], name: string): void; /** Indicates the tone of the choice list */ tone?: 'magic'; } -export function ChoiceList({ +export function ChoiceList({ title, titleHidden, allowMultiple, @@ -61,7 +61,7 @@ export function ChoiceList({ disabled = false, name: nameProp, tone, -}: ChoiceListProps) { +}: ChoiceListProps) { // Type asserting to any is required for TS3.2 but can be removed when we update to 3.3 // see https://github.com/Microsoft/TypeScript/issues/28768 const ControlComponent: any = allowMultiple ? Checkbox : RadioButton; @@ -97,7 +97,7 @@ export function ChoiceList({ ); } - const isSelected = choiceIsSelected(choice, selected); + const isSelected = selected.includes(choice.value); const renderedChildren = choice.renderChildren ? choice.renderChildren(isSelected) : null; @@ -116,7 +116,7 @@ export function ChoiceList({ label={label} disabled={choiceDisabled || disabled} fill={{xs: true, sm: false}} - checked={choiceIsSelected(choice, selected)} + checked={isSelected} helpText={helpText} onChange={handleChange} ariaDescribedBy={ @@ -154,14 +154,10 @@ export function ChoiceList({ function noop() {} -function choiceIsSelected({value}: Choice, selected: string[]) { - return selected.includes(value); -} - -function updateSelectedChoices( - {value}: Choice, +function updateSelectedChoices( + {value}: Choice, checked: boolean, - selected: string[], + selected: TValue[], allowMultiple = false, ) { if (checked) { From 419362e07d0c772636fbc169ac63e417e9dc06f6 Mon Sep 17 00:00:00 2001 From: Jason Addleman Date: Tue, 14 Nov 2023 15:33:24 -0500 Subject: [PATCH 2/3] Get rid of readonly array --- .../components/ChoiceList/ChoiceList.stories.tsx | 13 ++++++++----- .../src/components/ChoiceList/ChoiceList.tsx | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx b/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx index a4270268b61..c9fe9f72ea0 100644 --- a/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx +++ b/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx @@ -1,5 +1,6 @@ import React, {useCallback, useState} from 'react'; import type {ComponentMeta} from '@storybook/react'; +import type {ChoiceListProps} from '@shopify/polaris'; import {ChoiceList, TextField} from '@shopify/polaris'; export default { @@ -52,14 +53,16 @@ export function Magic() { 'hidden', ]); + const choices: ChoiceListProps['choices'] = [ + {label: 'Hidden', value: 'hidden'}, + {label: 'Optional', value: 'optional'}, + {label: 'Required', value: 'required'}, + ]; + return ( { /** Label for list of choices */ title: React.ReactNode; /** Collection of choices */ - choices: Choice[] | readonly Choice[]; + choices: Choice[]; /** Collection of selected choices */ selected: TValue[]; /** Name for form input */ From 817b4c265797bef9bc30b23fa6d6d39476ce2dcd Mon Sep 17 00:00:00 2001 From: Jason Addleman Date: Wed, 15 Nov 2023 08:33:25 -0500 Subject: [PATCH 3/3] Use generics --- .../src/components/ChoiceList/ChoiceList.stories.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx b/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx index c9fe9f72ea0..ff9fe92c861 100644 --- a/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx +++ b/polaris-react/src/components/ChoiceList/ChoiceList.stories.tsx @@ -29,8 +29,8 @@ export function Default() { } export function WithError() { - const [selected, setSelected] = useState([ - 'hidden' as HiddenOptionalRequired, + const [selected, setSelected] = useState([ + 'hidden', ]); return ( @@ -71,7 +71,7 @@ export function Magic() { } export function WithMultiChoice() { - const [selected, setSelected] = useState([] as string[]); + const [selected, setSelected] = useState([]); return (