From fd492b71699132c2997277ce848a93c88d8412bc Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 16 Aug 2024 18:09:22 +0200 Subject: [PATCH 1/8] Add some temporary test TODOs --- .../src/range-control/test/index.tsx | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/components/src/range-control/test/index.tsx b/packages/components/src/range-control/test/index.tsx index a4c5d8c6f2bc7f..9f627dd9035dcf 100644 --- a/packages/components/src/range-control/test/index.tsx +++ b/packages/components/src/range-control/test/index.tsx @@ -297,7 +297,25 @@ describe( 'RangeControl', () => { } ); } ); - describe( 'reset', () => { + describe.only( 'reset', () => { + // TODO: behaviour to be tested: + // - when resetFallbackValue is provided: + // - value resets to resetFallbackValue; + // - onChange() is called with resetFallbackValue; + // - when initialPosition is provided: + // - value is assigned to `null`; + // - onChange() is called with `undefined`; + // - value is usually assigned to initialPosition because the component + // is used in controlled mode and initialPosition is used as a fallback; + // - when neither resetFallbackValue nor initialPosition are provided: + // - value is assigned to `null`; + // - onChange() is called with `undefined`; + // - value remains `null` (empty) because there is no `initialPosition` + // to fallback to; + // - in any case, when the current value is the same as the reset value + // (ie. the value that would be assigned when clicking the reset button), + // the reset button should be disabled; + it( 'should reset to a custom fallback value, defined by a parent component', () => { const spy = jest.fn(); render( @@ -318,6 +336,8 @@ describe( 'RangeControl', () => { expect( rangeInput.value ).toBe( '33' ); expect( numberInput.value ).toBe( '33' ); expect( spy ).toHaveBeenCalledWith( 33 ); + + expect( resetButton ).toHaveAttribute( 'aria-disabled', 'true' ); } ); it( 'should reset to a 50% of min/max value, of no initialPosition or value is defined', () => { From c4bbaa4157c03803f928a814a850d9a3ae233296 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 16 Aug 2024 18:09:41 +0200 Subject: [PATCH 2/8] Temporarily comment out initialPosition's default value in Storybook --- packages/components/src/range-control/stories/index.story.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/range-control/stories/index.story.tsx b/packages/components/src/range-control/stories/index.story.tsx index 7775c364195722..863acec213a04f 100644 --- a/packages/components/src/range-control/stories/index.story.tsx +++ b/packages/components/src/range-control/stories/index.story.tsx @@ -72,7 +72,7 @@ export const Default: StoryFn< typeof RangeControl > = Template.bind( {} ); Default.args = { __nextHasNoMarginBottom: true, help: 'Please select how transparent you would like this.', - initialPosition: 50, + // initialPosition: 50, label: 'Opacity', max: 100, min: 0, From 73fe28522c69aac11856cb17c107bf92307c3cd8 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 16 Aug 2024 18:10:14 +0200 Subject: [PATCH 3/8] Tweak reset button disable logic, extract computeResetValue function --- .../components/src/range-control/index.tsx | 45 +++++++++++++++---- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/packages/components/src/range-control/index.tsx b/packages/components/src/range-control/index.tsx index 20d9712df9c251..3d23caeb7f1c75 100644 --- a/packages/components/src/range-control/index.tsx +++ b/packages/components/src/range-control/index.tsx @@ -41,6 +41,21 @@ import { space } from '../utils/space'; const noop = () => {}; +function computeResetValue( { + resetFallbackValue, + initialPosition, +}: Pick< RangeControlProps, 'resetFallbackValue' | 'initialPosition' > ) { + if ( resetFallbackValue !== undefined ) { + return ! Number.isNaN( resetFallbackValue ) ? resetFallbackValue : null; + } + + if ( initialPosition !== undefined ) { + return ! Number.isNaN( initialPosition ) ? initialPosition : null; + } + + return null; +} + function UnforwardedRangeControl( props: WordPressComponentProps< RangeControlProps, 'input', false >, forwardedRef: ForwardedRef< HTMLInputElement > @@ -166,13 +181,16 @@ function UnforwardedRangeControl( }; const handleOnReset = () => { - let resetValue: number | null = parseFloat( `${ resetFallbackValue }` ); - let onChangeResetValue: number | undefined = resetValue; + // Reset to `resetFallbackValue` if defined, otherwise set internal value + // to `null`. + const resetValue = Number.isNaN( resetFallbackValue ) + ? null + : resetFallbackValue ?? null; - if ( isNaN( resetValue ) ) { - resetValue = null; - onChangeResetValue = undefined; - } + // const resetValue = computeResetValue( { + // resetFallbackValue, + // initialPosition, + // } ); setValue( resetValue ); @@ -189,7 +207,7 @@ function UnforwardedRangeControl( * preserve the undefined callback argument, except when a * resetFallbackValue is defined. */ - onChange( onChangeResetValue ); + onChange( resetValue ?? undefined ); }; const handleShowTooltip = () => setShowTooltip( true ); @@ -210,6 +228,7 @@ function UnforwardedRangeControl( const offsetStyle = { [ isRTL() ? 'right' : 'left' ]: fillValueOffset, }; + return ( Date: Fri, 16 Aug 2024 18:37:03 +0200 Subject: [PATCH 4/8] Update test comments --- .../src/range-control/test/index.tsx | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/components/src/range-control/test/index.tsx b/packages/components/src/range-control/test/index.tsx index 9f627dd9035dcf..d08abd747c3703 100644 --- a/packages/components/src/range-control/test/index.tsx +++ b/packages/components/src/range-control/test/index.tsx @@ -298,20 +298,13 @@ describe( 'RangeControl', () => { } ); describe.only( 'reset', () => { - // TODO: behaviour to be tested: - // - when resetFallbackValue is provided: - // - value resets to resetFallbackValue; - // - onChange() is called with resetFallbackValue; - // - when initialPosition is provided: - // - value is assigned to `null`; - // - onChange() is called with `undefined`; - // - value is usually assigned to initialPosition because the component - // is used in controlled mode and initialPosition is used as a fallback; - // - when neither resetFallbackValue nor initialPosition are provided: - // - value is assigned to `null`; - // - onChange() is called with `undefined`; - // - value remains `null` (empty) because there is no `initialPosition` - // to fallback to; + // TODO: behaviour to be tested, in controlled mode: + // - when `resetFallbackValue` is provided, the value is ultimately set to + // `resetFallbackValue`; `onChange()` is called with `resetFallbackValue`. + // - when `initialPosition` is provided, the value is ultimately set to + // `initialPosition`; `onChange()` is called with `undefined`. + // - when neither `resetFallbackValue` nor `initialPosition` are provided, + // the input values is cleared; `onChange()` is called with `undefined`. // - in any case, when the current value is the same as the reset value // (ie. the value that would be assigned when clicking the reset button), // the reset button should be disabled; From ca92a21f463e832f44171152d822ff654e229b51 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 17 Aug 2024 21:48:56 +0200 Subject: [PATCH 5/8] Clean up code --- packages/components/src/range-control/index.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/components/src/range-control/index.tsx b/packages/components/src/range-control/index.tsx index 3d23caeb7f1c75..c9fbdc0055c855 100644 --- a/packages/components/src/range-control/index.tsx +++ b/packages/components/src/range-control/index.tsx @@ -41,6 +41,10 @@ import { space } from '../utils/space'; const noop = () => {}; +/** + * Computes the value that `RangeControl` should reset to when pressing + * the reset button. + */ function computeResetValue( { resetFallbackValue, initialPosition, @@ -182,16 +186,12 @@ function UnforwardedRangeControl( const handleOnReset = () => { // Reset to `resetFallbackValue` if defined, otherwise set internal value - // to `null`. + // to `null` — which, if propagated to the `value` prop, will cause + // the value to be reset to the `initialPosition` prop if defined. const resetValue = Number.isNaN( resetFallbackValue ) ? null : resetFallbackValue ?? null; - // const resetValue = computeResetValue( { - // resetFallbackValue, - // initialPosition, - // } ); - setValue( resetValue ); /** From 1872e89d3d281e70bd36e25721d5aecb0ebf47e5 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 17 Aug 2024 22:07:48 +0200 Subject: [PATCH 6/8] Update reset-related tests --- .../src/range-control/test/index.tsx | 71 ++++++++++++------- 1 file changed, 46 insertions(+), 25 deletions(-) diff --git a/packages/components/src/range-control/test/index.tsx b/packages/components/src/range-control/test/index.tsx index d08abd747c3703..3ce741867d0dbc 100644 --- a/packages/components/src/range-control/test/index.tsx +++ b/packages/components/src/range-control/test/index.tsx @@ -297,26 +297,38 @@ describe( 'RangeControl', () => { } ); } ); - describe.only( 'reset', () => { - // TODO: behaviour to be tested, in controlled mode: - // - when `resetFallbackValue` is provided, the value is ultimately set to - // `resetFallbackValue`; `onChange()` is called with `resetFallbackValue`. - // - when `initialPosition` is provided, the value is ultimately set to - // `initialPosition`; `onChange()` is called with `undefined`. - // - when neither `resetFallbackValue` nor `initialPosition` are provided, - // the input values is cleared; `onChange()` is called with `undefined`. - // - in any case, when the current value is the same as the reset value - // (ie. the value that would be assigned when clicking the reset button), - // the reset button should be disabled; - - it( 'should reset to a custom fallback value, defined by a parent component', () => { + describe( 'reset', () => { + it( 'should clear the input value when clicking the reset button', () => { + const spy = jest.fn(); + render( ); + + const resetButton = getResetButton(); + const rangeInput = getRangeInput(); + const numberInput = getNumberInput(); + + fireChangeEvent( numberInput, '14' ); + + expect( rangeInput.value ).toBe( '14' ); + expect( numberInput.value ).toBe( '14' ); + expect( spy ).toHaveBeenCalledWith( 14 ); + + fireEvent.click( resetButton ); + + // range input resets to min + (max-min)/2 + expect( rangeInput.value ).toBe( '50' ); + expect( numberInput.value ).toBe( '' ); + expect( spy ).toHaveBeenCalledWith( undefined ); + + expect( resetButton ).toHaveAttribute( 'aria-disabled', 'true' ); + } ); + + it( 'should reset to the `initialPosition` value when clicking the reset button', () => { const spy = jest.fn(); render( ); @@ -324,23 +336,29 @@ describe( 'RangeControl', () => { const rangeInput = getRangeInput(); const numberInput = getNumberInput(); + fireChangeEvent( numberInput, '14' ); + + expect( rangeInput.value ).toBe( '14' ); + expect( numberInput.value ).toBe( '14' ); + expect( spy ).toHaveBeenCalledWith( 14 ); + fireEvent.click( resetButton ); - expect( rangeInput.value ).toBe( '33' ); - expect( numberInput.value ).toBe( '33' ); - expect( spy ).toHaveBeenCalledWith( 33 ); + expect( rangeInput.value ).toBe( '23' ); + expect( numberInput.value ).toBe( '23' ); + expect( spy ).toHaveBeenCalledWith( undefined ); expect( resetButton ).toHaveAttribute( 'aria-disabled', 'true' ); } ); - it( 'should reset to a 50% of min/max value, of no initialPosition or value is defined', () => { + it( 'should reset to the `resetFallbackValue` value when clicking the reset button', () => { + const spy = jest.fn(); render( ); @@ -350,8 +368,11 @@ describe( 'RangeControl', () => { fireEvent.click( resetButton ); - expect( rangeInput.value ).toBe( '50' ); - expect( numberInput.value ).toBe( '' ); + expect( rangeInput.value ).toBe( '33' ); + expect( numberInput.value ).toBe( '33' ); + expect( spy ).toHaveBeenCalledWith( 33 ); + + expect( resetButton ).toHaveAttribute( 'aria-disabled', 'true' ); } ); } ); } ); From f7fb6fcc3ec92432ca71d462a359e67d505d47e8 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 17 Aug 2024 22:18:53 +0200 Subject: [PATCH 7/8] Restore Storybook default args --- packages/components/src/range-control/stories/index.story.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/range-control/stories/index.story.tsx b/packages/components/src/range-control/stories/index.story.tsx index 863acec213a04f..7775c364195722 100644 --- a/packages/components/src/range-control/stories/index.story.tsx +++ b/packages/components/src/range-control/stories/index.story.tsx @@ -72,7 +72,7 @@ export const Default: StoryFn< typeof RangeControl > = Template.bind( {} ); Default.args = { __nextHasNoMarginBottom: true, help: 'Please select how transparent you would like this.', - // initialPosition: 50, + initialPosition: 50, label: 'Opacity', max: 100, min: 0, From d0f46ee74b2c025f325b87426fdd42d2c83805d5 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 17 Aug 2024 22:23:58 +0200 Subject: [PATCH 8/8] CHANGELOG --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index f1b6ba3341de60..da44a5837bb816 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -59,6 +59,7 @@ ### Bug Fixes +- `RangeControl`: disable reset button when the current value is equal to the reset value ([#64579](https://github.com/WordPress/gutenberg/pull/64579)). - `RangeControl`: tweak mark and label absolute positioning ([#64487](https://github.com/WordPress/gutenberg/pull/64487)). ### Internal