From acaacd8c958b1aa0667c427b2fe210a9d0e0b6de Mon Sep 17 00:00:00 2001 From: arturbien Date: Sun, 30 Oct 2022 20:54:38 +0100 Subject: [PATCH] fix(slider): fix thumb not draggable fix #357 --- src/Slider/Slider.stories.tsx | 9 ++- src/Slider/Slider.tsx | 112 +++++++++++---------------- src/common/hooks/useEventCallback.ts | 23 ++++++ 3 files changed, 74 insertions(+), 70 deletions(-) create mode 100644 src/common/hooks/useEventCallback.ts diff --git a/src/Slider/Slider.stories.tsx b/src/Slider/Slider.stories.tsx index b7b96466..a002f3b2 100644 --- a/src/Slider/Slider.stories.tsx +++ b/src/Slider/Slider.stories.tsx @@ -1,6 +1,6 @@ import { ComponentMeta } from '@storybook/react'; import React from 'react'; -import { ScrollView, Slider } from 'react95'; +import { ScrollView, Slider, SliderOnChangeHandler } from 'react95'; import styled from 'styled-components'; const Wrapper = styled.div` @@ -41,6 +41,10 @@ export default { } as ComponentMeta; export function Default() { + const [state, setState] = React.useState(0); + + const onChange: SliderOnChangeHandler = (_, newValue) => setState(newValue); + return (
@@ -66,7 +70,8 @@ export function Default() { min={0} max={6} step={1} - defaultValue={0} + value={state} + onChange={onChange} marks={[ { value: 0, label: '0°C' }, { value: 2, label: '2°C' }, diff --git a/src/Slider/Slider.tsx b/src/Slider/Slider.tsx index 3be02071..ddf6ca89 100644 --- a/src/Slider/Slider.tsx +++ b/src/Slider/Slider.tsx @@ -17,12 +17,22 @@ import { createHatchedBackground } from '../common'; import useControlledOrUncontrolled from '../common/hooks/useControlledOrUncontrolled'; +import useEventCallback from '../common/hooks/useEventCallback'; import useForkRef from '../common/hooks/useForkRef'; import { useIsFocusVisible } from '../common/hooks/useIsFocusVisible'; import { clamp, getSize, roundValueToStep } from '../common/utils'; import { StyledScrollView } from '../ScrollView/ScrollView'; import { CommonStyledProps } from '../types'; +export type SliderOnChangeHandler = ( + event: + | MouseEvent + | React.KeyboardEvent + | React.MouseEvent + | TouchEvent, + value: number +) => void; + type SliderProps = { defaultValue?: number; disabled?: boolean; @@ -30,18 +40,8 @@ type SliderProps = { max?: number; min?: number; name?: string; - onChange?: ( - event: - | MouseEvent - | React.KeyboardEvent - | React.MouseEvent - | TouchEvent, - value: number - ) => void; - onChangeCommitted?: ( - event: MouseEvent | React.KeyboardEvent | TouchEvent, - value: number - ) => void; + onChange?: SliderOnChangeHandler; + onChangeCommitted?: SliderOnChangeHandler; onMouseDown?: (event: React.MouseEvent) => void; orientation?: 'horizontal' | 'vertical'; size?: string | number; @@ -330,21 +330,20 @@ const Slider = forwardRef( const handleFocusRef = useForkRef(focusVisibleRef, sliderRef); const handleRef = useForkRef(ref, handleFocusRef); - const handleFocus = useCallback( + const handleFocus = useEventCallback( (event: React.FocusEvent) => { if (isFocusVisible(event)) { setFocusVisible(true); } - }, - [isFocusVisible] + } ); - const handleBlur = useCallback(() => { + const handleBlur = useEventCallback(() => { if (focusVisible !== false) { setFocusVisible(false); onBlurVisible(); } - }, [focusVisible, onBlurVisible]); + }); const touchId = useRef(); @@ -363,7 +362,7 @@ const Slider = forwardRef( [marksProp, max, min, step] ); - const handleKeyDown = useCallback( + const handleKeyDown = useEventCallback( (event: React.KeyboardEvent) => { const tenPercents = (max - min) / 10; const marksValues = marks.map(mark => mark.value); @@ -422,17 +421,7 @@ const Slider = forwardRef( onChange?.(event, newValue); onChangeCommitted?.(event, newValue); - }, - [ - marks, - max, - min, - onChange, - onChangeCommitted, - setValueState, - step, - valueDerived - ] + } ); const getNewValue = useCallback( @@ -464,7 +453,7 @@ const Slider = forwardRef( [marks, max, min, step, vertical] ); - const handleTouchMove = useCallback( + const handleTouchMove = useEventCallback( (event: MouseEvent | TouchEvent) => { const finger = trackFinger(event, touchId.current); @@ -478,11 +467,10 @@ const Slider = forwardRef( setFocusVisible(true); onChange?.(event, newValue); - }, - [getNewValue, onChange, setValueState] + } ); - const handleTouchEnd = useCallback( + const handleTouchEnd = useEventCallback( (event: MouseEvent | TouchEvent) => { const finger = trackFinger(event, touchId.current); @@ -501,11 +489,10 @@ const Slider = forwardRef( doc.removeEventListener('mouseup', handleTouchEnd); doc.removeEventListener('touchmove', handleTouchMove); doc.removeEventListener('touchend', handleTouchEnd); - }, - [getNewValue, handleTouchMove, onChangeCommitted] + } ); - const handleMouseDown = useCallback( + const handleMouseDown = useEventCallback( (event: React.MouseEvent) => { // TODO should we also pass event together with new value to callbacks? (same thing with other input components) onMouseDown?.(event); @@ -524,43 +511,32 @@ const Slider = forwardRef( const doc = ownerDocument(sliderRef.current); doc.addEventListener('mousemove', handleTouchMove); doc.addEventListener('mouseup', handleTouchEnd); - }, - [ - getNewValue, - handleTouchEnd, - handleTouchMove, - onChange, - onMouseDown, - setValueState - ] + } ); - const handleTouchStart = useCallback( - (event: TouchEvent) => { - // Workaround as Safari has partial support for touchAction: 'none'. - event.preventDefault(); - const touch = event.changedTouches[0]; - if (touch != null) { - // A number that uniquely identifies the current finger in the touch session. - touchId.current = touch.identifier; - } + const handleTouchStart = useEventCallback((event: TouchEvent) => { + // Workaround as Safari has partial support for touchAction: 'none'. + event.preventDefault(); + const touch = event.changedTouches[0]; + if (touch != null) { + // A number that uniquely identifies the current finger in the touch session. + touchId.current = touch.identifier; + } - thumbRef.current?.focus(); - setFocusVisible(true); + thumbRef.current?.focus(); + setFocusVisible(true); - const finger = trackFinger(event, touchId.current); - if (finger) { - const newValue = getNewValue(finger); - setValueState(newValue); - onChange?.(event, newValue); - } + const finger = trackFinger(event, touchId.current); + if (finger) { + const newValue = getNewValue(finger); + setValueState(newValue); + onChange?.(event, newValue); + } - const doc = ownerDocument(sliderRef.current); - doc.addEventListener('touchmove', handleTouchMove); - doc.addEventListener('touchend', handleTouchEnd); - }, - [getNewValue, handleTouchEnd, handleTouchMove, onChange, setValueState] - ); + const doc = ownerDocument(sliderRef.current); + doc.addEventListener('touchmove', handleTouchMove); + doc.addEventListener('touchend', handleTouchEnd); + }); useEffect(() => { const { current: slider } = sliderRef; diff --git a/src/common/hooks/useEventCallback.ts b/src/common/hooks/useEventCallback.ts new file mode 100644 index 00000000..ceead553 --- /dev/null +++ b/src/common/hooks/useEventCallback.ts @@ -0,0 +1,23 @@ +import * as React from 'react'; + +const useEnhancedEffect = + typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect; + +/** + * https://github.com/facebook/react/issues/14099#issuecomment-440013892 + */ +export default function useEventCallback( + fn: (...args: Args) => Return +): (...args: Args) => Return { + const ref = React.useRef(fn); + useEnhancedEffect(() => { + ref.current = fn; + }); + return React.useCallback( + (...args: Args) => + // @ts-expect-error hide `this` + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + (0, ref.current!)(...args), + [] + ); +}