(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

@@ -21,7 +21,9 @@ export interface SliderCommon extends Omit<React_2.HTMLAttributes<HTMLDivElement
     disabled?: boolean;
     max?: number;
     min?: number;
-    onChange?: (value: number, ev?: React_2.PointerEvent<HTMLDivElement>) => void;
+    onChange?: (ev: React_2.PointerEvent<HTMLDivElement> | React_2.KeyboardEvent<HTMLDivElement>, data: {
+        value: number;
+    }) => void;
     origin?: number;
     step?: number;
     value?: number;

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<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement>,
+    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<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement>,
+    data: { value: number },
+  ) => setSliderValue(data.value);
 
   const styles = useStyles();
 

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 <Slider value={sliderValue} min={0} max={100} onChange={onChange} ref={sliderRef} />;
-    };
-
-    render(<SliderTestComponent />);
-
-    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', () => {

packages/react-slider/src/components/Slider/Slider.types.ts

@@ -104,7 +104,10 @@ export interface SliderCommon extends Omit<React.HTMLAttributes<HTMLDivElement>,
   /**
    * Triggers a callback when the value has been changed. This will be called on every individual step.
    */
-  onChange?: (value: number, ev?: React.PointerEvent<HTMLDivElement>) => void;
+  onChange?: (
+    ev: React.PointerEvent<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement>,
+    data: { value: number },
+  ) => void;
 
   /**
    * The **Slider's** current value label to be read by the screen reader.

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<SliderState, keyof SliderCommon | key
   const railRef = React.useRef<HTMLDivElement>(null);
   const thumbRef = React.useRef<HTMLDivElement>(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<SliderState, keyof SliderCommon | key
    * @param ev
    * @param incomingValue
    */
-  const updateValue = React.useCallback(
-    (incomingValue: number, ev): void => {
+  const updateValue = useEventCallback(
+    (incomingValue: number, ev: React.PointerEvent<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement>): void => {
       const clampedValue = clamp(incomingValue, min, max);
 
       if (clampedValue !== min && clampedValue !== max) {
@@ -74,13 +73,9 @@ export const useSliderState = (state: Pick<SliderState, keyof SliderCommon | key
         }
       }
 
-      if (onChange && onChangeCallback.current) {
-        onChangeCallback.current(clampedValue, ev);
-      }
-
+      onChange?.(ev, { value: clampedValue });
       setCurrentValue(clampedValue);
     },
-    [max, min, onChange, setCurrentValue],
   );
 
   /**
@@ -183,18 +178,6 @@ export const useSliderState = (state: Pick<SliderState, keyof SliderCommon | key
     }
   }, [currentValue, state.ref]);
 
-  useMount(() => {
-    // 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;