fix: onAfterChange should not trigger when click on track (#952)

* fix: onAfterChange should not trigger when click on track * fix: fix keyboard onAfterChange * chore: doc * chore: test case
react-component · Nov 30, 2023 · 99316a2 · 99316a2 · vercel · Nov 30, 2023
1 parent fa77b3e
commit 99316a2
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 7 deletions.
diff --git a/docs/examples/debug.tsx b/docs/examples/debug.tsx
@@ -52,6 +52,9 @@ export default () => {
             console.log('Change:', nextValues);
           //   setValue(nextValues as any);
           }}
+          onAfterChange={(v) => {
+            console.log('AfterChange:', v);
+          }}
           // value={value}
 
           min={0}

diff --git a/docs/examples/marks.tsx b/docs/examples/marks.tsx
@@ -25,7 +25,7 @@ export default () => (
   <div>
     <div style={style}>
       <p>Slider with marks, `step=null`</p>
-      <Slider min={-10} marks={marks} step={null} onChange={log} defaultValue={20} />
+      <Slider min={-10} marks={marks} step={null} onChange={log} defaultValue={20} onAfterChange={(v) => console.log('AfterChange:', v)} />
     </div>
 
     <div style={style}>

diff --git a/src/Handles/Handle.tsx b/src/Handles/Handle.tsx
@@ -23,6 +23,7 @@ export interface HandleProps {
   onFocus?: (e: React.FocusEvent<HTMLDivElement>) => void;
   onBlur?: (e: React.FocusEvent<HTMLDivElement>) => void;
   render?: (origin: React.ReactElement<HandleProps>, props: RenderProps) => React.ReactElement;
+  onChangeComplete?: () => void;
 }
 
 const Handle = React.forwardRef((props: HandleProps, ref: React.Ref<HTMLDivElement>) => {
@@ -35,6 +36,7 @@ const Handle = React.forwardRef((props: HandleProps, ref: React.Ref<HTMLDivEleme
     render,
     dragging,
     onOffsetChange,
+    onChangeComplete,
     ...restProps
   } = props;
   const {
@@ -109,6 +111,21 @@ const Handle = React.forwardRef((props: HandleProps, ref: React.Ref<HTMLDivEleme
     }
   };
 
+  const handleKeyUp = (e: React.KeyboardEvent<HTMLDivElement>) => {
+    switch (e.which || e.keyCode) {
+      case KeyCode.LEFT:
+      case KeyCode.RIGHT:
+      case KeyCode.UP:
+      case KeyCode.DOWN:
+      case KeyCode.HOME:
+      case KeyCode.END:
+      case KeyCode.PAGE_UP:
+      case KeyCode.PAGE_DOWN:
+        onChangeComplete?.();
+        break;
+    }
+  }
+
   // ============================ Offset ============================
   const positionStyle = getDirectionStyle(direction, value, min, max);
 
@@ -132,6 +149,7 @@ const Handle = React.forwardRef((props: HandleProps, ref: React.Ref<HTMLDivEleme
       onMouseDown={onInternalStartMove}
       onTouchStart={onInternalStartMove}
       onKeyDown={onKeyDown}
+      onKeyUp={handleKeyUp}
       tabIndex={disabled ? null : getIndex(tabIndex, valueIndex)}
       role="slider"
       aria-valuemin={min}

diff --git a/src/Handles/index.tsx b/src/Handles/index.tsx
@@ -14,6 +14,7 @@ export interface HandlesProps {
   onBlur?: (e: React.FocusEvent<HTMLDivElement>) => void;
   handleRender?: HandleProps['render'];
   draggingIndex: number;
+  onChangeComplete?: () => void;
 }
 
 export interface HandlesRef {

diff --git a/src/Slider.tsx b/src/Slider.tsx
@@ -329,7 +329,6 @@ const Slider = React.forwardRef((props: SliderProps, ref: React.Ref<SliderRef>)
 
       onBeforeChange?.(getTriggerValue(cloneNextValues));
       triggerChange(cloneNextValues);
-      onAfterChange?.(getTriggerValue(cloneNextValues));
       if (e) {
         onStartDrag(e, valueIndex, cloneNextValues);
       }
@@ -375,7 +374,6 @@ const Slider = React.forwardRef((props: SliderProps, ref: React.Ref<SliderRef>)
 
       onBeforeChange?.(getTriggerValue(rawValues));
       triggerChange(next.values);
-      onAfterChange?.(getTriggerValue(next.values));
 
       setKeyboardValue(next.value);
     }
@@ -543,6 +541,7 @@ const Slider = React.forwardRef((props: SliderProps, ref: React.Ref<SliderRef>)
           onFocus={onFocus}
           onBlur={onBlur}
           handleRender={handleRender}
+          onChangeComplete={finishChange}
         />
 
         <Marks prefixCls={prefixCls} marks={markList} onClick={changeToCloseValue} />

diff --git a/tests/Range.test.js b/tests/Range.test.js
@@ -116,8 +116,13 @@ describe('Range', () => {
     fireEvent.keyDown(container.getElementsByClassName('rc-slider-handle')[1], {
       keyCode: keyCode.RIGHT,
     });
+    expect(onAfterChange).not.toHaveBeenCalled();
 
-    expect(onAfterChange).toBeCalled();
+    fireEvent.keyUp(container.getElementsByClassName('rc-slider-handle')[1], {
+      keyCode: keyCode.RIGHT,
+    });
+
+    expect(onAfterChange).toHaveBeenCalled();
   });
 
   it('should not change value from keyboard events when disabled', () => {

diff --git a/tests/Slider.test.js b/tests/Slider.test.js
@@ -144,7 +144,13 @@ describe('Slider', () => {
       keyCode: keyCode.RIGHT,
     });
 
-    expect(onAfterChange).toBeCalled();
+    expect(onAfterChange).not.toHaveBeenCalled();
+
+    fireEvent.keyUp(container.getElementsByClassName('rc-slider-handle')[0], {
+      keyCode: keyCode.RIGHT,
+    });
+
+    expect(onAfterChange).toHaveBeenCalled();
   });
 
   it('decreases the value for reverse-horizontal when key "right" is pressed', () => {
@@ -182,13 +188,21 @@ describe('Slider', () => {
 
   it('decreases the value when key "left" is pressed', () => {
     const onChange = jest.fn();
-    const { container } = render(<Slider defaultValue={50} onChange={onChange} />);
+    const onChangeComplete = jest.fn();
+    const { container } = render(<Slider defaultValue={50} onChange={onChange} onAfterChange={onChangeComplete} />);
 
     fireEvent.keyDown(container.getElementsByClassName('rc-slider-handle')[0], {
       keyCode: keyCode.LEFT,
     });
 
     expect(onChange).toHaveBeenCalledWith(49);
+    expect(onChangeComplete).not.toHaveBeenCalled();
+
+    fireEvent.keyUp(container.getElementsByClassName('rc-slider-handle')[0], {
+      keyCode: keyCode.LEFT,
+    });
+
+    expect(onChangeComplete).toHaveBeenCalled();
   });
 
   it('it should work fine when arrow key is pressed', () => {
@@ -555,6 +569,10 @@ describe('Slider', () => {
 
       expect(onBeforeChange).toHaveBeenCalledWith(20);
       expect(onChange).toHaveBeenCalledWith(20);
+      expect(onAfterChange).not.toHaveBeenCalled();
+      fireEvent.mouseUp(container.querySelector('.rc-slider'), {
+        clientX: 20,
+      });
       expect(onAfterChange).toHaveBeenCalledWith(20);
     });
   });
@@ -607,6 +625,10 @@ describe('Slider', () => {
     fireEvent.mouseDown(container.querySelector('.rc-slider'), {
       clientX: 20,
     });
+    expect(onAfterChange).not.toHaveBeenCalled();
+    fireEvent.mouseUp(container.querySelector('.rc-slider'), {
+      clientX: 20,
+    });
     expect(onAfterChange).toHaveBeenCalledWith(20);
   });
 

diff --git a/tests/common.test.js b/tests/common.test.js
@@ -301,7 +301,10 @@ describe('Common', () => {
     );
     const sliderHandleWrapper = container.querySelector(`#${labelId}`);
     fireEvent.mouseDown(sliderHandleWrapper);
-    fireEvent.mouseUp(sliderHandleWrapper);
+    // Simulate propagation
+    fireEvent.mouseDown(container.querySelector('.rc-slider'));
+    fireEvent.mouseUp(container.querySelector('.rc-slider'));
+
     fireEvent.click(sliderHandleWrapper);
     expect(sliderOnChange).toHaveBeenCalled();
     expect(sliderOnAfterChange).toHaveBeenCalled();
@@ -312,6 +315,9 @@ describe('Common', () => {
     );
     const rangeHandleWrapper = container2.querySelector(`#${labelId}`);
     fireEvent.click(rangeHandleWrapper);
+    // Simulate propagation
+    fireEvent.mouseDown(container2.querySelector('.rc-slider'));
+    fireEvent.mouseUp(container2.querySelector('.rc-slider'));
     expect(rangeOnAfterChange).toHaveBeenCalled();
   });
 
@@ -327,6 +333,11 @@ describe('Common', () => {
     });
 
     expect(sliderOnChange).toHaveBeenCalled();
+    expect(sliderOnAfterChange).not.toHaveBeenCalled();
+
+    fireEvent.keyUp(container.getElementsByClassName('rc-slider-handle')[0], {
+      keyCode: KeyCode.UP,
+    });
     expect(sliderOnAfterChange).toHaveBeenCalled();
     expect(sliderOnAfterChange).toHaveBeenCalledTimes(1);
   });