Remove function set state from useControlledState (#2304)

devongovett · snowystinger · LFDanLu · web-flow · commit 10e57d449dde · 2022-01-10T17:21:57.000-08:00
* Remove function set state from useControlledState

* Fix lint and remaining issues

* fix TS

* Add a controlled story to test togglebutton

* more controlled stories

* fix checkbox group state

* fix lint

* add a controlled checkboxgroup story

* Update packages/@react-spectrum/button/stories/ToggleButton.stories.tsx

Co-authored-by: Kyle Taborski &lt;ktabors@yahoo.com&gt;

Co-authored-by: Robert Snow &lt;rsnow@adobe.com&gt;
Co-authored-by: Daniel Lu &lt;dlu@livefyre.com&gt;
Co-authored-by: Kyle Taborski &lt;ktabors@yahoo.com&gt;
diff --git a/packages/@react-aria/spinbutton/src/useSpinButton.ts b/packages/@react-aria/spinbutton/src/useSpinButton.ts
@@ -55,6 +55,8 @@ export function useSpinButton(
     onIncrementToMax
   } = props;
   const formatMessage = useMessageFormatter(intlMessages);
+  const propsRef = useRef(props);
+  propsRef.current = props;
 
   const clearAsync = () => clearTimeout(_async.current);
 
@@ -136,7 +138,7 @@ export function useSpinButton(
   const onIncrementPressStart = useCallback(
     (initialStepDelay: number) => {
       clearAsync();
-      onIncrement();
+      propsRef.current.onIncrement();
       // Start spinning after initial delay
       _async.current = window.setTimeout(
         () => {
@@ -153,7 +155,7 @@ export function useSpinButton(
   const onDecrementPressStart = useCallback(
     (initialStepDelay: number) => {
       clearAsync();
-      onDecrement();
+      propsRef.current.onDecrement();
       // Start spinning after initial delay
       _async.current = window.setTimeout(
         () => {
diff --git a/packages/@react-spectrum/accordion/stories/Accordion.stories.tsx b/packages/@react-spectrum/accordion/stories/Accordion.stories.tsx
@@ -12,7 +12,7 @@
 
 import {Story as _Story, Meta} from '@storybook/react';
 import {Accordion, Item} from '../src';
-import React from 'react';
+import React, {useState} from 'react';
 import {SpectrumAccordionProps} from '@react-types/accordion';
 
 type ItemType = {
@@ -62,10 +62,32 @@ const AccordionTemplate: Story<SpectrumAccordionProps<ItemType>> = (args) => (
   </Accordion>
 );
 
+
+const ControlledAccordionTemplate: Story<SpectrumAccordionProps<ItemType>> = (args) => {
+  let [openKeys, setOpenKeys] = useState<Set<React.Key>>(new Set(['files']));
+  return (
+    <Accordion {...args} expandedKeys={openKeys} onExpandedChange={setOpenKeys} >
+      <Item key="files" title="Your files">
+        files
+      </Item>
+      <Item key="shared" title="Shared with you">
+        shared
+      </Item>
+      <Item key="last" title="Last item">
+        last
+      </Item>
+    </Accordion>
+  );
+};
+
 export const DefaultExpandedKeys = AccordionTemplate.bind({});
 DefaultExpandedKeys.storyName = 'defaultExpandedKeys: files';
 DefaultExpandedKeys.args = {defaultExpandedKeys: ['files']};
 
+export const ControlledExpandedKeys = ControlledAccordionTemplate.bind({});
+ControlledExpandedKeys.storyName = 'controlled ExpandedKeys';
+ControlledExpandedKeys.args = {};
+
 export const DisabledKeys = AccordionTemplate.bind({});
 DisabledKeys.storyName = 'disabledKeys: files, shared';
 DisabledKeys.args = {disabledKeys: ['files', 'shared']};
diff --git a/packages/@react-spectrum/button/stories/ToggleButton.stories.tsx b/packages/@react-spectrum/button/stories/ToggleButton.stories.tsx
@@ -13,7 +13,7 @@
 import {action} from '@storybook/addon-actions';
 import Add from '@spectrum-icons/workflow/Add';
 import {Flex, Text, View} from '@adobe/react-spectrum';
-import React from 'react';
+import React, {useState} from 'react';
 import {storiesOf} from '@storybook/react';
 import {ToggleButton} from '../';
 
@@ -66,7 +66,10 @@ storiesOf('Button/ToggleButton', module)
         </Flex>
       </View>
     )
-  );
+  )
+  .add('controlled state', () => (
+    <ControlledToggleButton />
+  ));
 
 function render(props = {}) {
   return (<Flex gap="size-100">
@@ -84,3 +87,14 @@ function render(props = {}) {
     </ToggleButton>
   </Flex>);
 }
+
+function ControlledToggleButton() {
+  let [selected, setSelected] = useState(false);
+  return (
+    <div>
+      <ToggleButton isSelected={selected} onChange={setSelected}>Press Me</ToggleButton>
+      <br />
+      {selected ? 'true' : 'false'}
+    </div>
+  );
+}
diff --git a/packages/@react-spectrum/checkbox/stories/CheckboxGroup.stories.tsx b/packages/@react-spectrum/checkbox/stories/CheckboxGroup.stories.tsx
@@ -12,7 +12,7 @@
 
 import {action} from '@storybook/addon-actions';
 import {Checkbox, CheckboxGroup} from '../';
-import React from 'react';
+import React, {useState} from 'react';
 import {SpectrumCheckboxGroupProps} from '@react-types/checkbox';
 import {storiesOf} from '@storybook/react';
 
@@ -109,6 +109,10 @@ storiesOf('CheckboxGroup', module)
   .add(
     'form name',
     () => render({name: 'pets'})
+  )
+  .add(
+    'controlled',
+    () => <ControlledCheckboxGroup />
   );
 
 function render(props: Omit<SpectrumCheckboxGroupProps, 'children'> = {}, checkboxProps: any[] = []) {
@@ -120,3 +124,14 @@ function render(props: Omit<SpectrumCheckboxGroupProps, 'children'> = {}, checkb
     </CheckboxGroup>
   );
 }
+
+function ControlledCheckboxGroup() {
+  let [checked, setChecked] = useState([]);
+  return (
+    <CheckboxGroup label="Pets" onChange={setChecked} value={checked}>
+      <Checkbox value="dogs">Dogs</Checkbox>
+      <Checkbox value="cats">Cats</Checkbox>
+      <Checkbox value="dragons">Dragons</Checkbox>
+    </CheckboxGroup>
+  );
+}
diff --git a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx
@@ -22,7 +22,7 @@ import Cut from '@spectrum-icons/workflow/Cut';
 import {Item, Menu, MenuTrigger, Section} from '../';
 import {Keyboard, Text} from '@react-spectrum/text';
 import Paste from '@spectrum-icons/workflow/Paste';
-import React from 'react';
+import React, {useState} from 'react';
 import {storiesOf} from '@storybook/react';
 
 let iconMap = {
@@ -527,6 +527,9 @@ storiesOf('MenuTrigger', module)
         <Item key="3">Three</Item>
       </Menu>
     )
+  )
+  .add('controlled isOpen',
+    () => <ControlledOpeningMenuTrigger />
   );
 
 let customMenuItem = (item) => {
@@ -567,3 +570,25 @@ let defaultMenu = (
     )}
   </Menu>
 );
+
+function ControlledOpeningMenuTrigger() {
+  let [isOpen, setIsOpen] = useState(false);
+  return (
+    <div style={{display: 'flex', width: 'auto', margin: '250px 0'}}>
+      <MenuTrigger onOpenChange={setIsOpen} isOpen={isOpen}>
+        <ActionButton
+          onPress={action('press')}
+          onPressStart={action('pressstart')}
+          onPressEnd={action('pressend')}>
+          Menu Button
+        </ActionButton>
+        <Menu onAction={action('onAction')}>
+          <Item key="1">One</Item>
+          <Item key="">Two</Item>
+          <Item key="3">Three</Item>
+        </Menu>
+      </MenuTrigger>
+    </div>
+  );
+}
+
diff --git a/packages/@react-spectrum/numberfield/test/NumberField.test.js b/packages/@react-spectrum/numberfield/test/NumberField.test.js
@@ -1675,7 +1675,7 @@ describe('NumberField', function () {
     expect(onChangeSpy).toHaveBeenCalledWith(10123);
   });
 
-  it.skip.each`
+  it.each`
     Name
     ${'NumberField controlled'}
   `('$Name 10 is rendered and will change if the controlled version is implemented', () => {
diff --git a/packages/@react-stately/checkbox/src/useCheckboxGroupState.ts b/packages/@react-stately/checkbox/src/useCheckboxGroupState.ts
@@ -64,34 +64,27 @@ export function useCheckboxGroupState(props: CheckboxGroupProps = {}): CheckboxG
       if (props.isReadOnly || props.isDisabled) {
         return;
       }
-      setValue(values => {
-        if (!values.includes(value)) {
-          return values.concat(value);
-        }
-        return values;
-      });
+      if (!selectedValues.includes(value)) {
+        setValue(selectedValues.concat(value));
+      }
     },
     removeValue(value) {
       if (props.isReadOnly || props.isDisabled) {
         return;
       }
-      setValue(values => {
-        if (values.includes(value)) {
-          return values.filter(existingValue => existingValue !== value);
-        }
-        return values;
-      });
+      if (selectedValues.includes(value)) {
+        setValue(selectedValues.filter(existingValue => existingValue !== value));
+      }
     },
     toggleValue(value) {
       if (props.isReadOnly || props.isDisabled) {
         return;
       }
-      setValue(values => {
-        if (values.includes(value)) {
-          return values.filter(existingValue => existingValue !== value);
-        }
-        return values.concat(value);
-      });
+      if (selectedValues.includes(value)) {
+        setValue(selectedValues.filter(existingValue => existingValue !== value));
+      } else {
+        setValue(selectedValues.concat(value));
+      }
     }
   };
 
diff --git a/packages/@react-stately/checkbox/test/useCheckboxGroupState.test.tsx b/packages/@react-stately/checkbox/test/useCheckboxGroupState.test.tsx
@@ -228,7 +228,7 @@ describe('useCheckboxGroupState', () => {
       toggleValue('qwe');
     });
 
-    expect(container.textContent).toBe('foo, qwe');
+    expect(container.textContent).toBe('foo');
   });
 
   it('should not update state for readonly group', () => {
diff --git a/packages/@react-stately/color/src/useColorFieldState.ts b/packages/@react-stately/color/src/useColorFieldState.ts
@@ -74,20 +74,7 @@ export function useColorFieldState(
   let [colorValue, setColorValue] = useControlledState<Color>(initialValue, initialDefaultValue, onChange);
   let [inputValue, setInputValue] = useState(() => (value || defaultValue) && colorValue ? colorValue.toString('hex') : '');
 
-  let safelySetColorValue = (newColor: Color | ((prevState: Color) => Color)) => {
-    if (typeof newColor === 'function') {
-      setColorValue((prev:Color) => {
-        let resolved: Color = newColor(prev);
-        if (!prev || !resolved) {
-          return resolved;
-        }
-        if (resolved.toHexInt() !== prev.toHexInt()) {
-          return resolved;
-        }
-        return prev;
-      });
-      return;
-    }
+  let safelySetColorValue = (newColor: Color) => {
     if (!colorValue || !newColor) {
       setColorValue(newColor);
       return;
@@ -138,30 +125,26 @@ export function useColorFieldState(
   };
 
   let increment = () => {
-    safelySetColorValue((prevColor: Color) => {
-      let newValue = addColorValue(parsed.current, step);
-      // if we've arrived at the same value that was previously in the state, the
-      // input value should be updated to match
-      // ex type 4, press increment, highlight the number in the input, type 4 again, press increment
-      // you'd be at 5, then incrementing to 5 again, so no re-render would happen and 4 would be left in the input
-      if (newValue === prevColor) {
-        setInputValue(newValue.toString('hex'));
-      }
-      return newValue;
-    });
+    let newValue = addColorValue(parsed.current, step);
+    // if we've arrived at the same value that was previously in the state, the
+    // input value should be updated to match
+    // ex type 4, press increment, highlight the number in the input, type 4 again, press increment
+    // you'd be at 5, then incrementing to 5 again, so no re-render would happen and 4 would be left in the input
+    if (newValue === colorValue) {
+      setInputValue(newValue.toString('hex'));
+    }
+    safelySetColorValue(newValue);
   };
   let decrement = () => {
-    safelySetColorValue((prevColor: Color) => {
-      let newValue = addColorValue(parsed.current, -step);
-      // if we've arrived at the same value that was previously in the state, the
-      // input value should be updated to match
-      // ex type 4, press increment, highlight the number in the input, type 4 again, press increment
-      // you'd be at 5, then incrementing to 5 again, so no re-render would happen and 4 would be left in the input
-      if (newValue === prevColor) {
-        setInputValue(newValue.toString('hex'));
-      }
-      return newValue;
-    });
+    let newValue = addColorValue(parsed.current, -step);
+    // if we've arrived at the same value that was previously in the state, the
+    // input value should be updated to match
+    // ex type 4, press increment, highlight the number in the input, type 4 again, press increment
+    // you'd be at 5, then incrementing to 5 again, so no re-render would happen and 4 would be left in the input
+    if (newValue === colorValue) {
+      setInputValue(newValue.toString('hex'));
+    }
+    safelySetColorValue(newValue);
   };
   let incrementToMax = () => safelySetColorValue(MAX_COLOR);
   let decrementToMin = () => safelySetColorValue(MIN_COLOR);
diff --git a/packages/@react-stately/numberfield/src/useNumberFieldState.ts b/packages/@react-stately/numberfield/src/useNumberFieldState.ts
@@ -168,31 +168,27 @@ export function useNumberFieldState(
   };
 
   let increment = () => {
-    setNumberValue((previousValue) => {
-      let newValue = safeNextStep('+', minValue);
-
-      // if we've arrived at the same value that was previously in the state, the
-      // input value should be updated to match
-      // ex type 4, press increment, highlight the number in the input, type 4 again, press increment
-      // you'd be at 5, then incrementing to 5 again, so no re-render would happen and 4 would be left in the input
-      if (newValue === previousValue) {
-        setInputValue(format(newValue));
-      }
+    let newValue = safeNextStep('+', minValue);
+
+    // if we've arrived at the same value that was previously in the state, the
+    // input value should be updated to match
+    // ex type 4, press increment, highlight the number in the input, type 4 again, press increment
+    // you'd be at 5, then incrementing to 5 again, so no re-render would happen and 4 would be left in the input
+    if (newValue === numberValue) {
+      setInputValue(format(newValue));
+    }
 
-      return newValue;
-    });
+    setNumberValue(newValue);
   };
 
   let decrement = () => {
-    setNumberValue((previousValue) => {
-      let newValue = safeNextStep('-', maxValue);
+    let newValue = safeNextStep('-', maxValue);
 
-      if (newValue === previousValue) {
-        setInputValue(format(newValue));
-      }
+    if (newValue === numberValue) {
+      setInputValue(format(newValue));
+    }
 
-      return newValue;
-    });
+    setNumberValue(newValue);
   };
 
   let incrementToMax = () => {
diff --git a/packages/@react-stately/selection/src/types.ts b/packages/@react-stately/selection/src/types.ts
@@ -47,7 +47,7 @@ export interface MultipleSelectionState extends FocusState {
   /** The currently selected keys in the collection. */
   readonly selectedKeys: Selection,
   /** Sets the selected keys in the collection. */
-  setSelectedKeys(keys: Selection | ((v: Selection) => Selection)): void,
+  setSelectedKeys(keys: Selection): void,
   /** The currently disabled keys in the collection. */
   readonly disabledKeys: Set<Key>
 }
diff --git a/packages/@react-stately/toggle/src/useToggleState.ts b/packages/@react-stately/toggle/src/useToggleState.ts
diff --git a/packages/@react-stately/tree/src/useTreeState.ts b/packages/@react-stately/tree/src/useTreeState.ts
diff --git a/packages/@react-stately/utils/src/useControlledState.ts b/packages/@react-stately/utils/src/useControlledState.ts
diff --git a/packages/@react-stately/utils/test/useControlledState.test.js b/packages/@react-stately/utils/test/useControlledState.test.js
