From 18a6e17417bcb516701340b335c10ee3c7efa15a Mon Sep 17 00:00:00 2001 From: Simon Guo Date: Fri, 1 Dec 2023 11:01:59 +0800 Subject: [PATCH] =?UTF-8?q?fix(Slider,RangeSlider):=20fix=20onChangeCommit?= =?UTF-8?q?ted=20not=20triggered=20when=20the=E2=80=A6=20(#3472)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(Slider,RangeSlider): fix onChangeCommitted not triggered when the slider is clicked * test: add tests for disabled and readOnly * test: changed from assert syntax to expect * test: changed from assert syntax to expect --- src/RangeSlider/RangeSlider.tsx | 20 +++- src/RangeSlider/test/RangeSliderSpec.tsx | 134 ++++++++++++++++------- src/Slider/Slider.tsx | 12 +- src/Slider/test/SliderSpec.tsx | 89 +++++++++++---- 4 files changed, 187 insertions(+), 68 deletions(-) diff --git a/src/RangeSlider/RangeSlider.tsx b/src/RangeSlider/RangeSlider.tsx index d0c576003b..966f36f053 100644 --- a/src/RangeSlider/RangeSlider.tsx +++ b/src/RangeSlider/RangeSlider.tsx @@ -44,6 +44,7 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { progress = true, vertical, disabled, + readOnly, classPrefix = 'slider', min = 0, max: maxProp = 100, @@ -195,6 +196,10 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { * Callback function that is fired when the mousemove is triggered */ const handleDragMove = useEventCallback((event: React.MouseEvent, dataset: HandleDataset) => { + if (disabled || readOnly) { + return; + } + const nextValue = getNextValue(event, dataset); if (isRangeMatchingConstraint(nextValue)) { @@ -208,6 +213,10 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { */ const handleChangeCommitted = useCallback( (event: React.MouseEvent, dataset?: DOMStringMap) => { + if (disabled || readOnly) { + return; + } + const nextValue = getNextValue(event, dataset as HandleDataset); if (isRangeMatchingConstraint(nextValue)) { @@ -215,7 +224,7 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { onChangeCommitted?.(nextValue, event); } }, - [getNextValue, onChangeCommitted, isRangeMatchingConstraint, setValue] + [disabled, readOnly, getNextValue, isRangeMatchingConstraint, setValue, onChangeCommitted] ); const handleKeyDown = useCallback( @@ -264,9 +273,9 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { [max, min, onChange, rtl, isRangeMatchingConstraint, setValue, step, value] ); - const handleClick = useCallback( + const handleBarClick = useCallback( (event: React.MouseEvent) => { - if (disabled) { + if (disabled || readOnly) { return; } @@ -285,14 +294,17 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { if (isRangeMatchingConstraint(nextValue)) { setValue(nextValue); onChange?.(nextValue, event); + onChangeCommitted?.(nextValue, event); } }, [ disabled, + readOnly, getValidValue, getValueByPosition, isRangeMatchingConstraint, onChange, + onChangeCommitted, setValue, value ] @@ -340,7 +352,7 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => { return ( -
+
{progress && ( { it('Should render a RangeSlider', () => { const instance = getDOMNode(); - assert.equal(instance.className, 'rs-slider'); + expect(instance).to.have.class('rs-slider'); }); it('Should have a progress ', () => { const instance = getDOMNode(); - assert.equal( + + expect( // eslint-disable-next-line testing-library/no-node-access - (instance.querySelector('.rs-slider-progress-bar') as HTMLHtmlElement).style.width, - '40%' - ); - assert.equal( + (instance.querySelector('.rs-slider-progress-bar') as HTMLHtmlElement).style.width + ).to.equal('40%'); + + expect( // eslint-disable-next-line testing-library/no-node-access - (instance.querySelector('.rs-slider-progress-bar') as HTMLElement).style.left, - '10%' - ); + (instance.querySelector('.rs-slider-progress-bar') as HTMLElement).style.left + ).to.equal('10%'); }); it('Should render 2 handles ', () => { - const instance = getDOMNode(); - // eslint-disable-next-line testing-library/no-node-access - assert.equal(instance.querySelectorAll('.rs-slider-handle').length, 2); + render(); + expect(screen.getAllByRole('slider')).to.have.length(2); }); it('Should output the scale', () => { const instance = getDOMNode(); const instance2 = getDOMNode(); // eslint-disable-next-line testing-library/no-node-access - assert.equal(instance.querySelectorAll('li').length, 10); + expect(instance.querySelectorAll('li')).to.have.length(10); // eslint-disable-next-line testing-library/no-node-access - assert.equal(instance2.querySelectorAll('li').length, 9); + expect(instance2.querySelectorAll('li')).to.have.length(9); }); it('Should be displayed vertically', () => { const instance = getDOMNode(); - assert.include(instance.className, 'rs-slider-vertical'); + + expect(instance).to.have.class('rs-slider-vertical'); screen.getAllByRole('slider').forEach(slider => { expect(slider).to.have.attr('aria-orientation', 'vertical'); @@ -55,8 +55,38 @@ describe('RangeSlider', () => { }); it('Should be disabled', () => { - const instance = getDOMNode(); - assert.include(instance.className, 'rs-slider-disabled'); + const onChange = sinon.spy(); + const onChangeCommitted = sinon.spy(); + + const instance = getDOMNode( + + ); + expect(instance).to.have.class('rs-slider-disabled'); + expect(screen.queryAllByRole('slider')[0]).to.have.attr('aria-disabled', 'true'); + expect(screen.queryAllByRole('slider')[1]).to.have.attr('aria-disabled', 'true'); + + // eslint-disable-next-line testing-library/no-node-access + fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement); + + expect(onChange).to.have.not.been.called; + expect(onChangeCommitted).to.have.not.been.called; + }); + + it('Should be readOnly', () => { + const onChange = sinon.spy(); + const onChangeCommitted = sinon.spy(); + + const instance = getDOMNode( + + ); + expect(screen.queryAllByRole('slider')[0]).to.have.attr('readonly'); + expect(screen.queryAllByRole('slider')[1]).to.have.attr('readonly'); + + // eslint-disable-next-line testing-library/no-node-access + fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement); + + expect(onChange).to.have.not.been.called; + expect(onChangeCommitted).to.have.not.been.called; }); it('Should call onChange callback', () => { @@ -65,8 +95,7 @@ describe('RangeSlider', () => { // eslint-disable-next-line testing-library/no-node-access fireEvent.click(instance.querySelector('.rs-slider-progress-bar') as HTMLElement); - assert.equal(onChangeSpy.firstCall.firstArg[0], 0); - assert.equal(onChangeSpy.firstCall.firstArg[1], 50); + expect(onChangeSpy).to.have.been.calledWith([0, 50]); }); it('Should respond to keyboard event', async () => { @@ -107,7 +136,7 @@ describe('RangeSlider', () => { it('Should render custom title', () => { const instance = getDOMNode(); // eslint-disable-next-line testing-library/no-node-access - assert.equal((instance.querySelector('.rs-slider-handle') as HTMLElement).textContent, 'test'); + expect(instance.querySelector('.rs-slider-handle')).to.have.text('test'); }); it('Should handle keyboard operations', () => { @@ -116,25 +145,32 @@ describe('RangeSlider', () => { const handle = instance.querySelector('.rs-slider-handle') as HTMLElement; const input = screen.getAllByRole('slider')[0] as HTMLInputElement; - assert.equal(input.value, '10'); + expect(input).to.value('10'); + expect(input).to.have.attr('aria-valuenow', '10'); fireEvent.keyDown(handle, { key: 'ArrowUp' }); - assert.equal(input.value, '11'); + expect(input).to.value('11'); + expect(input).to.have.attr('aria-valuenow', '11'); fireEvent.keyDown(handle, { key: 'ArrowRight' }); - assert.equal(input.value, '12'); + expect(input).to.value('12'); + expect(input).to.have.attr('aria-valuenow', '12'); fireEvent.keyDown(handle, { key: 'ArrowDown' }); - assert.equal(input.value, '11'); + expect(input).to.value('11'); + expect(input).to.have.attr('aria-valuenow', '11'); fireEvent.keyDown(handle, { key: 'ArrowLeft' }); - assert.equal(input.value, '10'); + expect(input).to.value('10'); + expect(input).to.have.attr('aria-valuenow', '10'); fireEvent.keyDown(handle, { key: 'Home' }); - assert.equal(input.value, '0'); + expect(input).to.value('0'); + expect(input).to.have.attr('aria-valuenow', '0'); fireEvent.keyDown(handle, { key: 'End' }); - assert.equal(input.value, '100'); + expect(input).to.value('100'); + expect(input).to.have.attr('aria-valuenow', '100'); }); it('Should call `onChangeCommitted` callback', async () => { @@ -148,29 +184,40 @@ describe('RangeSlider', () => { fireEvent.mouseDown(handle); handle.dispatchEvent(mousemoveEvent); - assert.include(handle.className, 'active'); + expect(handle).to.have.class('active'); handle.dispatchEvent(mouseupEvent); expect(onChangeCommitted).to.have.been.calledOnce; }); + it('Should call `onChangeCommitted` callback when click bar', () => { + const onChangeCommitted = sinon.spy(); + const instance = getDOMNode( + + ); + // eslint-disable-next-line testing-library/no-node-access + fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement); + + expect(onChangeCommitted).to.have.been.calledWith([0, 50]); + }); + it('Should output an `input` stored value', () => { const instance = getDOMNode(); // eslint-disable-next-line testing-library/no-node-access const input = instance.querySelectorAll('input[type="range"]') as NodeListOf; - assert.equal(input[0].value, '20'); - assert.equal(input[0].getAttribute('aria-valuenow'), '20'); - assert.equal(input[0].getAttribute('aria-valuemax'), '100'); - assert.equal(input[0].getAttribute('aria-valuemin'), '10'); - assert.equal(input[0].getAttribute('aria-orientation'), 'horizontal'); - - assert.equal(input[1].value, '50'); - assert.equal(input[1].getAttribute('aria-valuenow'), '50'); - assert.equal(input[1].getAttribute('aria-valuemax'), '100'); - assert.equal(input[1].getAttribute('aria-valuemin'), '10'); - assert.equal(input[1].getAttribute('aria-orientation'), 'horizontal'); + expect(input[0]).to.value('20'); + expect(input[0]).to.have.attr('aria-valuenow', '20'); + expect(input[0]).to.have.attr('aria-valuemax', '100'); + expect(input[0]).to.have.attr('aria-valuemin', '10'); + expect(input[0]).to.have.attr('aria-orientation', 'horizontal'); + + expect(input[1]).to.value('50'); + expect(input[1]).to.have.attr('aria-valuenow', '50'); + expect(input[1]).to.have.attr('aria-valuemax', '100'); + expect(input[1]).to.have.attr('aria-valuemin', '10'); + expect(input[1]).to.have.attr('aria-orientation', 'horizontal'); }); it('Should be reversed start and end values', () => { @@ -191,16 +238,19 @@ describe('RangeSlider', () => { Simulate.click(sliderBar, { pageX: 0, pageY: 80 }); }); + expect(onChangeSpy).to.have.been.calledWith([20, 50]); + act(() => { Simulate.click(sliderBar, { pageX: 0, pageY: 0 }); }); - assert.deepEqual(onChangeSpy.firstCall.firstArg, [20, 50]); - /** * fix: https://github.com/rsuite/rsuite/issues/2425 * Error thrown before fix: expected [ 100, 20 ] to deeply equal [ 20, 100 ] */ - assert.deepEqual(onChangeSpy.secondCall.firstArg, [20, 100]); + + expect(onChangeSpy).to.have.been.calledWith([20, 100]); + + expect(onChangeSpy).to.have.been.calledTwice; }); }); diff --git a/src/Slider/Slider.tsx b/src/Slider/Slider.tsx index dd61ec9a1d..c9f59d51cc 100644 --- a/src/Slider/Slider.tsx +++ b/src/Slider/Slider.tsx @@ -142,7 +142,7 @@ const Slider = React.forwardRef((props: SliderProps, ref) => { const classes = merge( className, - withClassPrefix({ vertical, disabled, readOnly, graduated, 'with-mark': renderMark }) + withClassPrefix({ vertical, disabled, graduated, 'with-mark': renderMark, readonly: readOnly }) ); const max = useMemo( @@ -238,6 +238,14 @@ const Slider = React.forwardRef((props: SliderProps, ref) => { [disabled, getValidValue, getValueByPosition, onChangeCommitted, readOnly] ); + const handleClickBar = useCallback( + (event: React.MouseEvent) => { + handleChangeValue(event); + handleChangeCommitted(event); + }, + [handleChangeCommitted, handleChangeValue] + ); + const handleKeyDown = useCallback( (event: React.KeyboardEvent) => { let nextValue; @@ -283,7 +291,7 @@ const Slider = React.forwardRef((props: SliderProps, ref) => { return ( -
+
{progress && ( { it('Should render a Slider', () => { const instance = getDOMNode(); - assert.equal(instance.className, 'rs-slider'); + expect(instance).to.have.class('rs-slider'); }); it('Should have a progress ', () => { const instance = getDOMNode(); - assert.equal( - // eslint-disable-next-line testing-library/no-node-access - (instance.querySelector('.rs-slider-progress-bar') as HTMLElement).style.width, + + // eslint-disable-next-line testing-library/no-node-access + expect((instance.querySelector('.rs-slider-progress-bar') as HTMLElement).style.width).to.equal( '50%' ); }); @@ -26,9 +26,10 @@ describe('Slider', () => { const instance = getDOMNode(); const instance2 = getDOMNode(); // eslint-disable-next-line testing-library/no-node-access - assert.equal(instance.querySelectorAll('li').length, 10); + expect(instance.querySelectorAll('li')).to.have.length(10); + // eslint-disable-next-line testing-library/no-node-access - assert.equal(instance2.querySelectorAll('li').length, 9); + expect(instance2.querySelectorAll('li')).to.have.length(9); }); it('Should be displayed vertically', () => { @@ -38,8 +39,37 @@ describe('Slider', () => { }); it('Should be disabled', () => { - const instance = getDOMNode(); - assert.include(instance.className, 'rs-slider-disabled'); + const onChange = Sinon.spy(); + const onChangeCommitted = Sinon.spy(); + const instance = getDOMNode( + + ); + + expect(instance).to.have.class('rs-slider-disabled'); + expect(screen.getByRole('slider')).to.have.attr('aria-disabled', 'true'); + + // eslint-disable-next-line testing-library/no-node-access + fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement); + + expect(onChange).to.have.not.been.called; + expect(onChangeCommitted).to.have.not.been.called; + }); + + it('Should be readOnly', () => { + const onChange = Sinon.spy(); + const onChangeCommitted = Sinon.spy(); + const instance = getDOMNode( + + ); + + expect(instance).to.have.class('rs-slider-readonly'); + expect(screen.getByRole('slider')).to.have.attr('readonly'); + + // eslint-disable-next-line testing-library/no-node-access + fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement); + + expect(onChange).to.have.not.been.called; + expect(onChangeCommitted).to.have.not.been.called; }); it('Should custom render mark', () => { @@ -57,14 +87,15 @@ describe('Slider', () => { // eslint-disable-next-line testing-library/no-node-access const marks = instance.querySelectorAll('.rs-slider-mark-content'); - assert.equal(marks[0].textContent, 'Single'); - assert.equal(marks[1].textContent, '1'); - assert.equal(marks[2].textContent, '2'); + + expect(marks[0]).to.have.text('Single'); + expect(marks[1]).to.have.text('1'); + expect(marks[2]).to.have.text('2'); }); it('Should render custom title', () => { const instance = getDOMNode(); - assert.equal(instance.textContent, 'test'); + expect(instance).to.have.text('test'); }); it('Should handle keyboard operations', () => { @@ -73,25 +104,34 @@ describe('Slider', () => { const handle = instance.querySelector('.rs-slider-handle') as HTMLElement; // eslint-disable-next-line testing-library/no-node-access const input = instance.querySelector('input[type="range"]') as HTMLInputElement; - assert.equal(input.value, '10'); + + expect(input).to.value('10'); + expect(input).to.have.attr('aria-valuenow', '10'); fireEvent.keyDown(handle, { key: 'ArrowUp' }); - assert.equal(input.value, '11'); + + expect(input).to.value('11'); + expect(input).to.have.attr('aria-valuenow', '11'); fireEvent.keyDown(handle, { key: 'ArrowRight' }); - assert.equal(input.value, '12'); + expect(input).to.value('12'); + expect(input).to.have.attr('aria-valuenow', '12'); fireEvent.keyDown(handle, { key: 'ArrowDown' }); - assert.equal(input.value, '11'); + expect(input).to.value('11'); + expect(input).to.have.attr('aria-valuenow', '11'); fireEvent.keyDown(handle, { key: 'ArrowLeft' }); - assert.equal(input.value, '10'); + expect(input).to.value('10'); + expect(input).to.have.attr('aria-valuenow', '10'); fireEvent.keyDown(handle, { key: 'Home' }); - assert.equal(input.value, '0'); + expect(input).to.value('0'); + expect(input).to.have.attr('aria-valuenow', '0'); fireEvent.keyDown(handle, { key: 'End' }); - assert.equal(input.value, '100'); + expect(input).to.value('100'); + expect(input).to.have.attr('aria-valuenow', '100'); }); it('Should call `onChangeCommitted` callback', () => { @@ -105,12 +145,21 @@ describe('Slider', () => { fireEvent.mouseDown(handle); handle.dispatchEvent(mousemoveEvent); - assert.include(handle.className, 'active'); + expect(handle).to.have.class('active'); handle.dispatchEvent(mouseupEvent); expect(onChangeCommitted).to.have.been.calledOnce; }); + it('Should call `onChangeCommitted` callback when click bar', () => { + const onChangeCommitted = Sinon.spy(); + const instance = getDOMNode(); + // eslint-disable-next-line testing-library/no-node-access + fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement); + + expect(onChangeCommitted).to.have.been.calledOnce; + }); + it('Should call `onChange` callback', () => { const onChange = Sinon.spy(); const instance = getDOMNode();