From 52b71df328ffadf0aa48385fdb6cbe6157f86154 Mon Sep 17 00:00:00 2001 From: Caleb Zearing Date: Mon, 30 Nov 2020 11:40:09 -0800 Subject: [PATCH] Fix issues with SpinButton revealed by eslint-plugin-react-hooks (#15431) --- ...at-react-internal-spinButton-onChange.json | 8 + .../react-internal/etc/react-internal.api.md | 2 +- .../components/SpinButton/SpinButton.base.tsx | 490 ++++++------ .../components/SpinButton/SpinButton.test.tsx | 711 +++++++++--------- .../components/SpinButton/SpinButton.types.ts | 2 +- .../__snapshots__/SpinButton.test.tsx.snap | 4 +- 6 files changed, 601 insertions(+), 616 deletions(-) create mode 100644 change/@fluentui-react-internal-2020-10-08-11-29-41-feat-react-internal-spinButton-onChange.json diff --git a/change/@fluentui-react-internal-2020-10-08-11-29-41-feat-react-internal-spinButton-onChange.json b/change/@fluentui-react-internal-2020-10-08-11-29-41-feat-react-internal-spinButton-onChange.json new file mode 100644 index 00000000000000..e99d94432f62fa --- /dev/null +++ b/change/@fluentui-react-internal-2020-10-08-11-29-41-feat-react-internal-spinButton-onChange.json @@ -0,0 +1,8 @@ +{ + "type": "prerelease", + "comment": "SpinButton: Fix function component conversion issues revealed by eslint-plugin-react-hooks", + "packageName": "@fluentui/react-internal", + "email": "czearing@outlook.com", + "dependentChangeType": "patch", + "date": "2020-10-08T18:29:41.503Z" +} diff --git a/packages/react-internal/etc/react-internal.api.md b/packages/react-internal/etc/react-internal.api.md index 702073729e238e..1702af307bc879 100644 --- a/packages/react-internal/etc/react-internal.api.md +++ b/packages/react-internal/etc/react-internal.api.md @@ -4594,7 +4594,7 @@ export interface ISpinButton { } // @public (undocumented) -export interface ISpinButtonProps extends React.HTMLAttributes, React.RefAttributes { +export interface ISpinButtonProps extends React.HTMLAttributes, React.RefAttributes { ariaDescribedBy?: string; ariaLabel?: string; ariaPositionInSet?: number; diff --git a/packages/react-internal/src/components/SpinButton/SpinButton.base.tsx b/packages/react-internal/src/components/SpinButton/SpinButton.base.tsx index 8d01c2a16ff180..d75074a5a5a679 100644 --- a/packages/react-internal/src/components/SpinButton/SpinButton.base.tsx +++ b/packages/react-internal/src/components/SpinButton/SpinButton.base.tsx @@ -1,37 +1,45 @@ -/* eslint-disable react-hooks/exhaustive-deps -- will come back and fix separately */ import * as React from 'react'; import { IconButton } from '../../compat/Button'; import { Label } from '../../Label'; import { Icon } from '../../Icon'; import { - getId, KeyCodes, calculatePrecision, classNamesFunction, precisionRound, getNativeProps, + getPropsWithDefaults, divProperties, } from '../../Utilities'; import { getArrowButtonStyles } from './SpinButton.styles'; import { ISpinButtonProps, ISpinButtonStyleProps, ISpinButtonStyles, KeyboardSpinDirection } from './SpinButton.types'; import { Position } from '../../Positioning'; -import { useBoolean, useSetTimeout, useControllableValue, useWarnings } from '@fluentui/react-hooks'; +import { useSetTimeout, useControllableValue, useWarnings, useId } from '@fluentui/react-hooks'; interface ISpinButtonInternalState { - inputId: string; - labelId: string; - lastValidValue: string; - spinningByMouse: boolean; - valueToValidate: string | undefined; - precision: number; - currentStepFunctionHandle: number; - initialStepDelay: number; - stepDelay: number; + lastValidValue?: string; + spinningByMouse?: boolean; + valueToValidate?: string; + stepTimeoutHandle: number; } const getClassNames = classNamesFunction(); const COMPONENT_NAME = 'SpinButton'; +const DEFAULT_PROPS = { + disabled: false, + label: '', + min: 0, + max: 100, + step: 1, + labelPosition: Position.start, + incrementButtonIcon: { iconName: 'ChevronUpSmall' }, + decrementButtonIcon: { iconName: 'ChevronDownSmall' }, +}; +type ISpinButtonPropsWithDefaults = ISpinButtonProps & Required>; + +const INITIAL_STEP_DELAY = 400; +const STEP_DELAY = 75; const useComponentRef = ( props: ISpinButtonProps, @@ -50,25 +58,41 @@ const useComponentRef = ( } }, }), - [value], + [input, value], ); }; +const onChange = (): void => { + /** + * A noop input change handler. Using onInput instead of onChange was meant to address an issue + * which apparently has been resolved in React 16 (https://github.com/facebook/react/issues/7027). + * The no-op onChange handler was still needed because React gives console errors if an input + * doesn't have onChange. + * + * TODO (Fabric 8?) - switch to just calling onChange (this is a breaking change for any tests, + * ours or 3rd-party, which simulate entering text in a SpinButton) + */ +}; + export const SpinButtonBase: React.FunctionComponent = React.forwardRef< HTMLDivElement, ISpinButtonProps ->((props, ref) => { +>((propsWithoutDefaults, ref) => { + const props = getPropsWithDefaults(DEFAULT_PROPS, propsWithoutDefaults) as ISpinButtonPropsWithDefaults; const { - disabled = false, - label = '', - min = 0, - max = 100, - step = 1, - labelPosition = Position.start, + disabled, + label, + min, + max, + step, + defaultValue, + value: valueFromProps, + precision: precisionFromProps, + labelPosition, iconProps, - incrementButtonIcon = { iconName: 'ChevronUpSmall' }, + incrementButtonIcon, incrementButtonAriaLabel, - decrementButtonIcon = { iconName: 'ChevronDownSmall' }, + decrementButtonIcon, decrementButtonAriaLabel, ariaLabel, ariaDescribedBy, @@ -81,56 +105,36 @@ export const SpinButtonBase: React.FunctionComponent = React.f ariaValueText, className, inputProps, + onDecrement, + onIncrement, iconButtonProps, + onValidate, styles, - } = props as ISpinButtonProps; + } = props; const input = React.useRef(null); - const [isFocused, { setTrue: setTrueIsFocused, setFalse: setFalseIsFocused }] = useBoolean(false); + const inputId = useId('input'); + const labelId = useId('Label'); + + const [isFocused, setIsFocused] = React.useState(false); const [keyboardSpinDirection, setKeyboardSpinDirection] = React.useState(KeyboardSpinDirection.notSpinning); const { setTimeout, clearTimeout } = useSetTimeout(); - const [value, setValue] = useControllableValue( - props.value, - props.defaultValue !== undefined ? props.defaultValue : String(min || '0'), - ); - if (process.env.NODE_ENV !== 'production') { - // eslint-disable-next-line react-hooks/rules-of-hooks -- build-time conditional - useWarnings({ - name: COMPONENT_NAME, - props, - mutuallyExclusive: { value: 'defaultValue' }, - }); - } - - useComponentRef(props, input, value); - const callCalculatePrecision = (calculatePrecisionProps: ISpinButtonProps) => { - const { precision = Math.max(calculatePrecision(calculatePrecisionProps.step!), 0) } = calculatePrecisionProps; - return precision; - }; - - let { value: initialValue = props.defaultValue } = props; - if (initialValue === undefined) { - initialValue = typeof min === 'number' ? String(min) : '0'; - } + const [value, setValue] = useControllableValue(valueFromProps, defaultValue ?? String(min)); const { current: internalState } = React.useRef({ - inputId: getId('input'), - labelId: getId('Label'), - lastValidValue: initialValue, - spinningByMouse: false, - valueToValidate: undefined, - precision: callCalculatePrecision(props as ISpinButtonProps), - currentStepFunctionHandle: -1, - initialStepDelay: 400, - stepDelay: 75, + lastValidValue: value, + stepTimeoutHandle: -1, }); - if (props.value !== undefined) { - internalState.lastValidValue = props.value; + if (typeof valueFromProps === 'string') { + // Ensure that we respect updates to props.value when determining the last valid value + internalState.lastValidValue = valueFromProps; } - internalState.precision = callCalculatePrecision(props as ISpinButtonProps); + const precision = React.useMemo(() => { + return precisionFromProps ?? Math.max(calculatePrecision(step), 0); + }, [precisionFromProps, step]); const classNames = getClassNames(styles, { theme: theme!, @@ -147,28 +151,18 @@ export const SpinButtonBase: React.FunctionComponent = React.f 'className', ]); - // Validate function to use if one is not passed in - const defaultOnValidate = (valueProp: string) => { - if (valueProp === null || valueProp.trim().length === 0 || isNaN(Number(valueProp))) { + /** Composed validation handler (uses `props.onValidate` or default) */ + const handleValidate = (newValue: string, ev: React.SyntheticEvent) => { + if (onValidate) { + return onValidate(newValue, ev); + } + if (!newValue || newValue.trim().length === 0 || isNaN(Number(newValue))) { return internalState.lastValidValue; } - const newValue = Math.min(max as number, Math.max(min as number, Number(valueProp))); - return String(newValue); + return String(Math.min(max, Math.max(min, Number(newValue)))); }; - // Increment function to use if one is not passed in - const defaultOnIncrement = (valueProp: string): string | void => { - let newValue: number = Math.min(Number(valueProp) + Number(step), max); - newValue = precisionRound(newValue, internalState.precision); - return String(newValue); - }; - - // Increment function to use if one is not passed in - const defaultOnDecrement = (valueProp: string): string | void => { - let newValue: number = Math.max(Number(valueProp) - Number(step), min); - newValue = precisionRound(newValue, internalState.precision); - return String(newValue); - }; + /** Validate function called on blur or enter keypress. */ const validate = (ev: React.FocusEvent | React.KeyboardEvent): void => { if ( @@ -176,7 +170,8 @@ export const SpinButtonBase: React.FunctionComponent = React.f internalState.valueToValidate !== undefined && internalState.valueToValidate !== internalState.lastValidValue ) { - const newValue = onValidate!(internalState.valueToValidate, ev); + const newValue = handleValidate(internalState.valueToValidate, ev); + // Done validating this value, so clear it internalState.valueToValidate = undefined; if (newValue !== undefined) { @@ -189,189 +184,187 @@ export const SpinButtonBase: React.FunctionComponent = React.f } }; - const onIncrement = (valueProp: string): string | void => { - if (props.onIncrement) { - return props.onIncrement(valueProp); - } else { - return defaultOnIncrement(valueProp); + /** + * Stop spinning (clear any currently pending update and set spinning to false) + */ + const stop = React.useCallback((): void => { + if (internalState.stepTimeoutHandle >= 0) { + clearTimeout(internalState.stepTimeoutHandle); + internalState.stepTimeoutHandle = -1; } - }; - - const onDecrement = (valueProp: string): string | void => { - if (props.onDecrement) { - return props.onDecrement(valueProp); - } else { - return defaultOnDecrement(valueProp); + if (internalState.spinningByMouse || keyboardSpinDirection !== KeyboardSpinDirection.notSpinning) { + internalState.spinningByMouse = false; + setKeyboardSpinDirection(KeyboardSpinDirection.notSpinning); } - }; + }, [internalState, keyboardSpinDirection, clearTimeout]); - const onValidate = (valueProp: string, ev?: React.SyntheticEvent): string | void => { - if (props.onValidate) { - return props.onValidate(valueProp, ev); - } else { - return defaultOnValidate(valueProp); - } - }; + /** + * Update the value with the given stepFunction + * @param stepFunction - function to use to step by + * @param event - The event that triggered the updateValue + * @param shouldSpin - should we fire off another updateValue when we are done here? This should be true + * when spinning in response to a mousedown. It doesn't need to be true for keyboard spinning since + * holding the key will automatically fire more keydown events. + */ + const updateValue = React.useCallback( + ( + stepFunction: ( + value: string, + event?: React.MouseEvent | React.KeyboardEvent, + ) => string | void, + ev?: React.MouseEvent | React.KeyboardEvent, + shouldSpin: boolean = false, + ): void => { + setValue(prevValue => { + // For spinning by mouse, where additional updateValue calls are triggered via setTimeout, + // we must access the current value in an updater callback (rather than referencing `value` + // directly) because otherwise we'd be reusing the stale captured value from the first call + // and spinning wouldn't work. (The other possible approach here is storing the value in + // internalState while spinning, then setting the actual state when spinning stops.) + const newValue = stepFunction(prevValue || '', ev); + if (newValue !== undefined) { + internalState.lastValidValue = newValue; + return newValue; + } + return prevValue; + }); - const onChange = React.useCallback((): void => { - /** - * A noop input change handler. Using onInput instead of onChange was meant to address an issue - * which apparently has been resolved in React 16 (https://github.com/facebook/react/issues/7027). - * The no-op onChange handler was still needed because React gives console errors if an input - * doesn't have onChange. - * - * TODO (Fabric 8?) - switch to just calling onChange (this is a breaking change for any tests, - * ours or 3rd-party, which simulate entering text in a SpinButton) - */ - }, []); - - // The method is needed to ensure we are updating the actual input value. - // without this our value will never change (and validation will not have the correct number) - const onInputChange = React.useCallback((event: React.FormEvent): void => { - const element: HTMLInputElement = event.target as HTMLInputElement; - const elementValue: string = element.value; - internalState.valueToValidate = elementValue; - setValue(elementValue); - }, []); + const wasSpinning = internalState.spinningByMouse; + internalState.spinningByMouse = shouldSpin; - const onFocus = React.useCallback( - (ev: React.FocusEvent): void => { - // We can't set focus on a non-existing element - if (!input.current) { - return; + if (shouldSpin) { + internalState.stepTimeoutHandle = setTimeout( + () => { + updateValue(stepFunction, ev, true); + }, + wasSpinning ? STEP_DELAY : INITIAL_STEP_DELAY, + ); } - if (internalState.spinningByMouse || keyboardSpinDirection !== KeyboardSpinDirection.notSpinning) { - stop(); - } - input.current.select(); - setTrueIsFocused(); - if (props.onFocus) { - props.onFocus(ev); + }, + [internalState, setValue, setTimeout], + ); + + /** Composed increment handler (uses `props.onIncrement` or default) */ + const handleIncrement = React.useCallback( + (newValue: string): string | void => { + if (onIncrement) { + return onIncrement(newValue); + } else { + let numericValue: number = Math.min(Number(newValue) + Number(step), max); + numericValue = precisionRound(numericValue, precision); + return String(numericValue); } }, - [props.onFocus], + [precision, max, onIncrement, step], ); - const onBlur = React.useCallback( - (ev: React.FocusEvent): void => { - validate(ev); - setFalseIsFocused(); - if (props.onBlur) { - props.onBlur(ev); + /** Composed decrement handler (uses `props.onDecrement` or default) */ + const handleDecrement = React.useCallback( + (newValue: string): string | void => { + if (onDecrement) { + return onDecrement(newValue); + } else { + let numericValue: number = Math.max(Number(newValue) - Number(step), min); + numericValue = precisionRound(numericValue, precision); + return String(numericValue); } }, - [props.onBlur], + [precision, min, onDecrement, step], ); - /** - * Update the value with the given stepFunction - * @param shouldSpin - should we fire off another updateValue when we are done here? This should be true - * when spinning in response to a mouseDown - * @param stepFunction - function to use to step by - * @param event - The event that triggered the updateValue - */ - const updateValue = ( - shouldSpin: boolean, - stepDelay: number, - stepFunction: ( - value: string, - event?: React.MouseEvent | React.KeyboardEvent, - ) => string | void, - event?: React.MouseEvent | React.KeyboardEvent, - ): void => { - const newValue: string | void = stepFunction(value || '', event); - if (newValue !== undefined) { - setValue(newValue); - } + /** Handles when the user types in the input */ + const handleInputChange = (ev: React.FormEvent): void => { + const elementValue = (ev.target as HTMLInputElement).value; + internalState.valueToValidate = elementValue; + setValue(elementValue); + }; - if (internalState.spinningByMouse !== shouldSpin) { - internalState.spinningByMouse = shouldSpin; + /** Composed focus handler (does internal stuff and calls `props.onFocus`) */ + const handleFocus = (ev: React.FocusEvent): void => { + // We can't set focus on a non-existing element + if (!input.current) { + return; } - - if (shouldSpin) { - internalState.currentStepFunctionHandle = setTimeout(() => { - updateValue(shouldSpin, internalState.stepDelay, stepFunction, event); - }, stepDelay); + if (internalState.spinningByMouse || keyboardSpinDirection !== KeyboardSpinDirection.notSpinning) { + stop(); } + input.current.select(); + setIsFocused(true); + props.onFocus?.(ev); }; - // Stop spinning (clear any currently pending update and set spinning to false) - const stop = React.useCallback((): void => { - if (internalState.currentStepFunctionHandle >= 0) { - clearTimeout(internalState.currentStepFunctionHandle); - internalState.currentStepFunctionHandle = -1; + /** Composed blur handler (does internal stuff and calls `props.onBlur`) */ + const handleBlur = (ev: React.FocusEvent): void => { + validate(ev); + setIsFocused(false); + props.onBlur?.(ev); + }; + + /** Update value when arrow keys are pressed, commit on enter, or revert on escape */ + const handleKeyDown = (ev: React.KeyboardEvent): void => { + // eat the up and down arrow keys to keep focus in the spinButton + // (especially when a spinButton is inside of a FocusZone) + if (ev.which === KeyCodes.up || ev.which === KeyCodes.down || ev.which === KeyCodes.enter) { + ev.preventDefault(); + ev.stopPropagation(); } - if (internalState.spinningByMouse || keyboardSpinDirection !== KeyboardSpinDirection.notSpinning) { - internalState.spinningByMouse = false; - setKeyboardSpinDirection(KeyboardSpinDirection.notSpinning); + if (disabled) { + stop(); + return; } - }, [internalState.currentStepFunctionHandle, keyboardSpinDirection, KeyboardSpinDirection.notSpinning]); - - // Handle keydown on the text field. We need to update - // the value when up or down arrow are depressed - const handleKeyDown = React.useCallback( - (event: React.KeyboardEvent): void => { - // eat the up and down arrow keys to keep focus in the spinButton - // (especially when a spinButton is inside of a FocusZone) - if (event.which === KeyCodes.up || event.which === KeyCodes.down || event.which === KeyCodes.enter) { - event.preventDefault(); - event.stopPropagation(); - } - if (props.disabled) { - stop(); - return; - } - let spinDirection = KeyboardSpinDirection.notSpinning; - - switch (event.which) { - case KeyCodes.up: - spinDirection = KeyboardSpinDirection.up; - updateValue(false /* shouldSpin */, internalState.initialStepDelay, onIncrement!, event); - break; - case KeyCodes.down: - spinDirection = KeyboardSpinDirection.down; - updateValue(false /* shouldSpin */, internalState.initialStepDelay, onDecrement!, event); - break; - case KeyCodes.enter: - validate(event); - break; - case KeyCodes.escape: - if (value !== internalState.lastValidValue) { - setValue(internalState.lastValidValue); - } - break; - default: - break; - } - // style the increment/decrement button to look active - // when the corresponding up/down arrow keys trigger a step - if (keyboardSpinDirection !== spinDirection) { - setKeyboardSpinDirection(spinDirection); - } - }, - [KeyCodes], - ); + let spinDirection = KeyboardSpinDirection.notSpinning; + + switch (ev.which) { + case KeyCodes.up: + spinDirection = KeyboardSpinDirection.up; + updateValue(handleIncrement, ev); + break; + case KeyCodes.down: + spinDirection = KeyboardSpinDirection.down; + updateValue(handleDecrement, ev); + break; + case KeyCodes.enter: + validate(ev); + break; + case KeyCodes.escape: + setValue(internalState.lastValidValue); + break; + } + // style the increment/decrement button to look active + // when the corresponding up/down arrow keys trigger a step + if (keyboardSpinDirection !== spinDirection) { + setKeyboardSpinDirection(spinDirection); + } + }; - // Make sure that we have stopped spinning on keyUp - // if the up or down arrow fired this event + /** Stop spinning on keyUp if the up or down arrow key fired this event */ const handleKeyUp = React.useCallback( (ev: React.KeyboardEvent): void => { - if (props.disabled || ev.which === KeyCodes.up || ev.which === KeyCodes.down) { + if (disabled || ev.which === KeyCodes.up || ev.which === KeyCodes.down) { stop(); return; } }, - [props.disabled], + [disabled, stop], + ); + + const handleIncrementMouseDown = React.useCallback( + (ev: React.MouseEvent): void => { + updateValue(handleIncrement, ev, true /* shouldSpin */); + }, + [handleIncrement, updateValue], ); - const onIncrementMouseDown = React.useCallback((event: React.MouseEvent): void => { - updateValue(true /* shouldSpin */, internalState.initialStepDelay, onIncrement!, event); - }, []); + const handleDecrementMouseDown = React.useCallback( + (ev: React.MouseEvent): void => { + updateValue(handleDecrement, ev, true /* shouldSpin */); + }, + [handleDecrement, updateValue], + ); - const onDecrementMouseDown = React.useCallback((event: React.MouseEvent): void => { - updateValue(true /* shouldSpin */, internalState.initialStepDelay, onDecrement!, event); - }, []); + useComponentRef(props, input, value); + useDebugWarnings(props); return (
@@ -379,12 +372,7 @@ export const SpinButtonBase: React.FunctionComponent = React.f
{iconProps &&