From 004f0e573411a9c63418bdbc9377bea91cf121a5 Mon Sep 17 00:00:00 2001 From: John Coburn Date: Fri, 12 Jul 2024 14:53:29 -0500 Subject: [PATCH] consolidate onChange calls (#2322) --- lib/Selection/Selection.js | 43 +++++++++---------- lib/Selection/tests/Selection-test.js | 23 +++++++--- lib/Selection/tests/SingleSelectionHarness.js | 4 +- 3 files changed, 41 insertions(+), 29 deletions(-) diff --git a/lib/Selection/Selection.js b/lib/Selection/Selection.js index 70c542a8b..60fe0d051 100644 --- a/lib/Selection/Selection.js +++ b/lib/Selection/Selection.js @@ -1,7 +1,7 @@ import React, { useState, useMemo, useEffect, useCallback, useRef } from 'react'; import PropTypes from 'prop-types'; import { useIntl } from 'react-intl'; -import { UseComboboxStateChangeTypes, useCombobox } from 'downshift'; +import { useCombobox } from 'downshift'; import classNames from 'classnames'; import isEqual from 'lodash/isEqual'; import formField from '../FormField'; @@ -120,11 +120,9 @@ const Selection = ({ }) => { const { formatMessage } = useIntl(); const [filterValue, updateFilterValue] = useState(''); - const [selectedItem, updateSelectedItem] = useState(value ? getSelectedObject(value, dataOptions) : null); const dataLength = useRef(dataOptions?.length || 0); const controlRef = useProvidedRefOrCreate(inputRef); const awaitingChange = useRef(false); - const chosenItem = useRef(null); const options = useMemo( () => (asyncFilter ? dataOptions : filterValue ? onFilter(filterValue, dataOptions) : dataOptions), @@ -133,15 +131,6 @@ const Selection = ({ const testId = useProvidedIdOrCreate(id, 'selection-'); - useEffect(() => { - // if dataOptions populate after the initial render, update the selectedItem state - // if one hasn't been found yet. - if (dataOptions?.length !== dataLength.current && value && selectedItem === null) { - updateSelectedItem(getSelectedObject(value, dataOptions)); - dataLength.current = dataOptions.length; - } - }, [dataOptions, selectedItem, value]) - // we need to skip over group headings since those can neither be selectable or cursored over. const reducedListItems = reduceOptions(options); const { @@ -152,14 +141,17 @@ const Selection = ({ getMenuProps, highlightedIndex, getItemProps, + selectedItem, + selectItem: updateSelectedItem, } = useCombobox({ items: reducedListItems, itemToString: defaultItemToString, - selectedItem, + initialSelectedItem: value ? getSelectedObject(value, dataOptions) : null, onSelectedItemChange: ({ selectedItem: newSelectedItem }) => { - if (onChange) { + // if the newSelectedItem's value matches the incoming value prop, we assume that + // onChange isn't necessary. + if (onChange && newSelectedItem?.value !== value) { onChange(newSelectedItem.value); - updateSelectedItem(newSelectedItem); } }, isItemDisabled(item) { @@ -171,7 +163,6 @@ const Selection = ({ case useCombobox.stateChangeTypes.InputKeyDownEnter: case useCombobox.stateChangeTypes.ItemClick: awaitingChange.current = true; - chosenItem.current = changes.selectedItem; return changes; default: return changes; @@ -179,23 +170,31 @@ const Selection = ({ } }); + useEffect(() => { + // if dataOptions populate/change after the initial render, update the selectedItem state + // if one hasn't been found yet. + if (dataOptions?.length !== dataLength.current && value && selectedItem === null) { + const selected = getSelectedObject(value, dataOptions); + updateSelectedItem(selected); + dataLength.current = dataOptions.length; + } + }, [dataOptions, selectedItem, value]) + const valueLabel = defaultItemToString(selectedItem) || placeholder || ''; const labelId = `sl-label-${testId}`; const valueId = `selected-${testId}-item`; if (awaitingChange.current) { - if (chosenItem.current !== null && chosenItem.current?.value === value) { + // a user has change the value via the dropdown and we're waiting for the value + // to correspond with the state... + if (selectedItem !== null && selectedItem?.value === value) { awaitingChange.current = false; - chosenItem.current = null; } } else if (selectedItem !== null && selectedItem?.value !== value){ - // conform to post-render value prop changes from outside of the component, + // conform to post-mount value prop changes from outside of the component, // whether the changed value is something empty like '' or null; const newValue = getSelectedObject(value, dataOptions) || { value } updateSelectedItem(newValue); - if (onChange) { - onChange(newValue.value); - } } /** renderOptions diff --git a/lib/Selection/tests/Selection-test.js b/lib/Selection/tests/Selection-test.js index 873786aea..d8410c9dc 100644 --- a/lib/Selection/tests/Selection-test.js +++ b/lib/Selection/tests/Selection-test.js @@ -577,29 +577,32 @@ describe('Selection', () => { describe('Changing the value prop outside of render', () => { const changeSpy = sinon.spy(); + const harnessHandler = (fn, val) => { + changeSpy(val); + fn(val); + }; beforeEach(async () => { + changeSpy.resetHistory(); await mountWithContext( ); }); it('renders initial value as provided', () => selection.has({ singleValue: 'Option 2' })); - describe('Reseting the value', () => { + describe('Resetting the value outside of the component', () => { beforeEach(async () => { await Button('reset').click(); }); - it('removes the value', () => selection.has({ singleValue: '' })) + it('removes the value', () => selection.has({ singleValue: '' })); - it('calls the supplied change handler', async () => { - await expect(changeSpy.calledOnceWith('')); - }) + it('will not trigger a onChange', async () => converge(() => { if (changeSpy.called) throw new Error('expected change handler to be called once') })); }); }); @@ -633,6 +636,14 @@ describe('Selection', () => { it('updates the value', () => selection.has({ singleValue: 'Option 0' })); it('calls the supplied change handler', async () => converge(() => { if (!changeSpy.calledOnceWith('test0')) throw new Error('expected change handler to be called') })); + + describe('additional renders...', () => { + beforeEach(async () => { + await Button(including('update')).click(); + }); + + it('will not trigger an additional onChange', async () => converge(() => { if (!changeSpy.calledOnce) throw new Error('expected change handler to be called once') })); + }); }); }); diff --git a/lib/Selection/tests/SingleSelectionHarness.js b/lib/Selection/tests/SingleSelectionHarness.js index a53a39449..bb019a88f 100644 --- a/lib/Selection/tests/SingleSelectionHarness.js +++ b/lib/Selection/tests/SingleSelectionHarness.js @@ -10,11 +10,13 @@ const SingleSelectionHarness = ({ onChange = (fn, val) => { fn(val) }, }) => { const [fieldVal, setFieldVal] = useState(initValue); - const [options, updateOptions] = useState(optionsProp) + const [options, updateOptions] = useState(optionsProp); + const [increment, updateIncrement] = useState(0); return ( <> + ;