From e6d5658f610c047a7d575f328da142e01cec0bc8 Mon Sep 17 00:00:00 2001 From: Caleb Zearing Date: Wed, 18 Aug 2021 09:10:57 -0700 Subject: [PATCH] (react-slider) - Updating onChange typing (#19412) * Updating onChange in Slider component * Switching to useEventCallback and updating typing. * Updating the onChange if condition to a single line * PR feedback --- packages/react-slider/etc/react-slider.api.md | 4 ++- packages/react-slider/src/Slider.stories.tsx | 10 ++++-- .../src/components/Slider/Slider.test.tsx | 34 +++---------------- .../src/components/Slider/Slider.types.ts | 5 ++- .../src/components/Slider/useSliderState.tsx | 25 +++----------- 5 files changed, 23 insertions(+), 55 deletions(-) diff --git a/packages/react-slider/etc/react-slider.api.md b/packages/react-slider/etc/react-slider.api.md index dcb21f27febfc0..f4d172883ef2e0 100644 --- a/packages/react-slider/etc/react-slider.api.md +++ b/packages/react-slider/etc/react-slider.api.md @@ -21,7 +21,9 @@ export interface SliderCommon extends Omit) => void; + onChange?: (ev: React_2.PointerEvent | React_2.KeyboardEvent, data: { + value: number; + }) => void; origin?: number; step?: number; value?: number; diff --git a/packages/react-slider/src/Slider.stories.tsx b/packages/react-slider/src/Slider.stories.tsx index ceb81fb8cecb83..66af91e138b4d1 100644 --- a/packages/react-slider/src/Slider.stories.tsx +++ b/packages/react-slider/src/Slider.stories.tsx @@ -20,7 +20,10 @@ const useStyles = makeStyles({ export const BasicSliderExample = (props: SliderProps) => { const [sliderValue, setSliderValue] = React.useState(160); - const sliderOnChange = (value: number) => setSliderValue(value); + const sliderOnChange = ( + ev: React.PointerEvent | React.KeyboardEvent, + data: { value: number }, + ) => setSliderValue(data.value); const styles = useStyles(); @@ -42,7 +45,10 @@ export const BasicSliderExample = (props: SliderProps) => { export const VerticalSliderExample = (props: SliderProps) => { const [sliderValue, setSliderValue] = React.useState(160); - const sliderOnChange = (value: number) => setSliderValue(value); + const sliderOnChange = ( + ev: React.PointerEvent | React.KeyboardEvent, + data: { value: number }, + ) => setSliderValue(data.value); const styles = useStyles(); diff --git a/packages/react-slider/src/components/Slider/Slider.test.tsx b/packages/react-slider/src/components/Slider/Slider.test.tsx index 57bd16834a3ffe..71779bd59b9caf 100644 --- a/packages/react-slider/src/components/Slider/Slider.test.tsx +++ b/packages/react-slider/src/components/Slider/Slider.test.tsx @@ -121,32 +121,6 @@ describe('Slider', () => { expect(sliderRef.current.value).toEqual(0); }); - it('clamps provided controlled (value) that is out of bounds', () => { - let sliderRef: any; - let imperativeRef: any; - - const SliderTestComponent = () => { - const [sliderValue, setSliderValue] = React.useState(-10); - sliderRef = React.useRef(null); - imperativeRef = React.useRef(null); - - React.useImperativeHandle(imperativeRef, () => ({ - getSliderValue: () => { - return sliderValue; - }, - })); - - const onChange = (value: number) => setSliderValue(value); - - return ; - }; - - render(); - - expect(sliderRef.current.value).toEqual(0); - expect(imperativeRef.current.getSliderValue()).toEqual(0); - }); - it('calls (onChange) when dragged', () => { let sliderRef: any; const onChange = jest.fn(); @@ -164,7 +138,7 @@ describe('Slider', () => { fireEvent.pointerDown(sliderRoot, { clientX: 0, clientY: 0 }); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange.mock.calls[0][0]).toEqual(0); + expect(onChange.mock.calls[0][1]).toEqual({ value: 0 }); expect(sliderRef.current.value).toBe(0); }); @@ -188,13 +162,13 @@ describe('Slider', () => { sliderRoot.simulate('pointerdown', { type: 'pointermove', clientX: 110, clientY: 0 }); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange.mock.calls[0][0]).toEqual(10); + expect(onChange.mock.calls[0][1]).toEqual({ value: 10 }); expect(sliderRef.current.value).toBe(10); sliderRoot.simulate('pointerdown', { type: 'pointermove', clientX: -10, clientY: 0 }); expect(onChange).toHaveBeenCalledTimes(2); - expect(onChange.mock.calls[1][0]).toEqual(0); + expect(onChange.mock.calls[1][1]).toEqual({ value: 0 }); expect(sliderRef.current.value).toBe(0); wrapper.unmount(); @@ -284,7 +258,7 @@ describe('Slider', () => { fireEvent.keyDown(sliderRoot, { key: 'ArrowUp' }); expect(sliderRef.current.value).toEqual(50); - expect(onChange.mock.calls[2][0]).toEqual(51); + expect(onChange.mock.calls[2][1]).toEqual({ value: 51 }); }); it('handles a negative (step) prop', () => { diff --git a/packages/react-slider/src/components/Slider/Slider.types.ts b/packages/react-slider/src/components/Slider/Slider.types.ts index ce0bf5e06564b6..a99e7f2bd64c00 100644 --- a/packages/react-slider/src/components/Slider/Slider.types.ts +++ b/packages/react-slider/src/components/Slider/Slider.types.ts @@ -104,7 +104,10 @@ export interface SliderCommon extends Omit, /** * Triggers a callback when the value has been changed. This will be called on every individual step. */ - onChange?: (value: number, ev?: React.PointerEvent) => void; + onChange?: ( + ev: React.PointerEvent | React.KeyboardEvent, + data: { value: number }, + ) => void; /** * The **Slider's** current value label to be read by the screen reader. diff --git a/packages/react-slider/src/components/Slider/useSliderState.tsx b/packages/react-slider/src/components/Slider/useSliderState.tsx index c2d8f9e1f72c36..da7c7b2c281712 100644 --- a/packages/react-slider/src/components/Slider/useSliderState.tsx +++ b/packages/react-slider/src/components/Slider/useSliderState.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useId, useControllableState, useMount } from '@fluentui/react-utilities'; +import { useId, useControllableState, useEventCallback } from '@fluentui/react-utilities'; import { SliderSlots, SliderState, SliderCommon } from './Slider.types'; /** @@ -54,7 +54,6 @@ export const useSliderState = (state: Pick(null); const thumbRef = React.useRef(null); const disposables = React.useRef<(() => void)[]>([]); - const onChangeCallback = React.useRef(onChange); const id = useId('slider-', state.id); /** @@ -63,8 +62,8 @@ export const useSliderState = (state: Pick { + const updateValue = useEventCallback( + (incomingValue: number, ev: React.PointerEvent | React.KeyboardEvent): void => { const clampedValue = clamp(incomingValue, min, max); if (clampedValue !== min && clampedValue !== max) { @@ -74,13 +73,9 @@ export const useSliderState = (state: Pick { - // If the user passes an out of bounds controlled value, clamp and update their value onMount. - if (value !== undefined && (value < min || value > max) && onChange && onChangeCallback.current) { - onChangeCallback.current(clamp(value, min, max)); - - if (process.env.NODE_ENV !== 'production') { - // eslint-disable-next-line no-console - console.warn('It appears that a controlled Slider has received an unclamped value outside of the min/max.'); - } - } - }); - const valuePercent = getPercent(currentValue!, min, max); const originPercent = origin ? getPercent(origin, min, max) : 0;