From ba5cecef32d89438310fb8aa40f11d7e76c7b3ae Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 4 Jan 2024 18:13:01 +0100 Subject: [PATCH 1/7] ColorPicker: store internal HSLA state for better slider UX --- .../components/src/color-picker/hsl-input.tsx | 86 ++++++++++++------- 1 file changed, 56 insertions(+), 30 deletions(-) diff --git a/packages/components/src/color-picker/hsl-input.tsx b/packages/components/src/color-picker/hsl-input.tsx index 3331a97b3d4de..2929fc3cea883 100644 --- a/packages/components/src/color-picker/hsl-input.tsx +++ b/packages/components/src/color-picker/hsl-input.tsx @@ -3,6 +3,11 @@ */ import { colord } from 'colord'; +/** + * WordPress dependencies + */ +import { useState, useEffect } from '@wordpress/element'; + /** * Internal dependencies */ @@ -10,7 +15,49 @@ import { InputWithSlider } from './input-with-slider'; import type { HslInputProps } from './types'; export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { - const { h, s, l, a } = color.toHsl(); + const colorPropHSL = color.toHsl(); + + const [ internalHSLA, setInternalHSLA ] = useState( { ...colorPropHSL } ); + + const isInternalColorSameAsReceivedColor = color.isEqual( + colord( internalHSLA ) + ); + + useEffect( () => { + if ( ! isInternalColorSameAsReceivedColor ) { + setInternalHSLA( colorPropHSL ); + } + }, [ colorPropHSL, isInternalColorSameAsReceivedColor ] ); + + const colorValue = isInternalColorSameAsReceivedColor + ? internalHSLA + : colorPropHSL; + + const updateHSLAValue = ( + partialNewValue: Partial< typeof colorPropHSL > + ) => { + setInternalHSLA( ( prevValue ) => ( { + ...prevValue, + ...partialNewValue, + } ) ); + + const nextOnChangeValue = colord( { + ...colorValue, + ...partialNewValue, + } ); + + // Avoid firing `onChange` if the resulting didn't change. + if ( color.isEqual( nextOnChangeValue ) ) { + return; + } + + onChange( + colord( { + ...colorValue, + ...partialNewValue, + } ) + ); + }; return ( <> @@ -19,9 +66,9 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { max={ 359 } label="Hue" abbreviation="H" - value={ h } + value={ colorValue.h } onChange={ ( nextH: number ) => { - onChange( colord( { h: nextH, s, l, a } ) ); + updateHSLAValue( { h: nextH } ); } } /> { max={ 100 } label="Saturation" abbreviation="S" - value={ s } + value={ colorValue.s } onChange={ ( nextS: number ) => { - onChange( - colord( { - h, - s: nextS, - l, - a, - } ) - ); + updateHSLAValue( { s: nextS } ); } } /> { max={ 100 } label="Lightness" abbreviation="L" - value={ l } + value={ colorValue.l } onChange={ ( nextL: number ) => { - onChange( - colord( { - h, - s, - l: nextL, - a, - } ) - ); + updateHSLAValue( { l: nextL } ); } } /> { enableAlpha && ( @@ -64,16 +97,9 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { max={ 100 } label="Alpha" abbreviation="A" - value={ Math.trunc( 100 * a ) } + value={ Math.trunc( 100 * colorValue.a ) } onChange={ ( nextA: number ) => { - onChange( - colord( { - h, - s, - l, - a: nextA / 100, - } ) - ); + updateHSLAValue( { a: nextA / 100 } ); } } /> ) } From 2b126da1f44ff862828cdbe9c83ec1b0527495b1 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 4 Jan 2024 23:48:31 +0100 Subject: [PATCH 2/7] CHANGELOG --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index c7c1a515a64ce..0b5feb1203b98 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -23,6 +23,7 @@ ### Enhancements +- `ColorPicker`: improve the UX around HSL sliders ([#57555](https://github.com/WordPress/gutenberg/pull/57555)). - Update `ariakit` to version `0.3.10` ([#57325](https://github.com/WordPress/gutenberg/pull/57325)). - Update `@ariakit/react` to version `0.3.12` and @ariakit/test to version `0.3.7` ([#57547](https://github.com/WordPress/gutenberg/pull/57547)). - `DropdownMenuV2`: do not collapse suffix width ([#57238](https://github.com/WordPress/gutenberg/pull/57238)). From 20953b2fdee267858586018767a5c96b2533f39f Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 5 Jan 2024 09:35:57 +0100 Subject: [PATCH 3/7] Comments --- .../components/src/color-picker/hsl-input.tsx | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/components/src/color-picker/hsl-input.tsx b/packages/components/src/color-picker/hsl-input.tsx index 2929fc3cea883..ccc66d7cc5e95 100644 --- a/packages/components/src/color-picker/hsl-input.tsx +++ b/packages/components/src/color-picker/hsl-input.tsx @@ -25,10 +25,15 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { useEffect( () => { if ( ! isInternalColorSameAsReceivedColor ) { + // Keep internal HSLA color up to date with the received color prop setInternalHSLA( colorPropHSL ); } }, [ colorPropHSL, isInternalColorSameAsReceivedColor ] ); + // If the internal color is equal to the received color prop, we can use the + // HSLA values from the local state which, compared to the received color prop, + // retain more details about the actual H and S values that the user selected, + // and thus allow for better UX when interacting with the H and S sliders. const colorValue = isInternalColorSameAsReceivedColor ? internalHSLA : colorPropHSL; @@ -36,6 +41,7 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { const updateHSLAValue = ( partialNewValue: Partial< typeof colorPropHSL > ) => { + // Update internal HSLA color. setInternalHSLA( ( prevValue ) => ( { ...prevValue, ...partialNewValue, @@ -46,17 +52,11 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { ...partialNewValue, } ); - // Avoid firing `onChange` if the resulting didn't change. - if ( color.isEqual( nextOnChangeValue ) ) { - return; + // Fire `onChange` only if the resulting color is different from the + // current one. + if ( ! color.isEqual( nextOnChangeValue ) ) { + onChange( nextOnChangeValue ); } - - onChange( - colord( { - ...colorValue, - ...partialNewValue, - } ) - ); }; return ( From d4c34dcc00c453f25dfb780c3380e7013f6677d9 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 10 Jan 2024 15:51:12 +0100 Subject: [PATCH 4/7] Memoize `colorPropHSL`, rename to `colorPropHSLA` --- packages/components/src/color-picker/hsl-input.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/components/src/color-picker/hsl-input.tsx b/packages/components/src/color-picker/hsl-input.tsx index ccc66d7cc5e95..118677becde4e 100644 --- a/packages/components/src/color-picker/hsl-input.tsx +++ b/packages/components/src/color-picker/hsl-input.tsx @@ -6,7 +6,7 @@ import { colord } from 'colord'; /** * WordPress dependencies */ -import { useState, useEffect } from '@wordpress/element'; +import { useState, useEffect, useMemo } from '@wordpress/element'; /** * Internal dependencies @@ -15,9 +15,9 @@ import { InputWithSlider } from './input-with-slider'; import type { HslInputProps } from './types'; export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { - const colorPropHSL = color.toHsl(); + const colorPropHSLA = useMemo( () => color.toHsl(), [ color ] ); - const [ internalHSLA, setInternalHSLA ] = useState( { ...colorPropHSL } ); + const [ internalHSLA, setInternalHSLA ] = useState( { ...colorPropHSLA } ); const isInternalColorSameAsReceivedColor = color.isEqual( colord( internalHSLA ) @@ -26,9 +26,9 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { useEffect( () => { if ( ! isInternalColorSameAsReceivedColor ) { // Keep internal HSLA color up to date with the received color prop - setInternalHSLA( colorPropHSL ); + setInternalHSLA( colorPropHSLA ); } - }, [ colorPropHSL, isInternalColorSameAsReceivedColor ] ); + }, [ colorPropHSLA, isInternalColorSameAsReceivedColor ] ); // If the internal color is equal to the received color prop, we can use the // HSLA values from the local state which, compared to the received color prop, @@ -36,10 +36,10 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { // and thus allow for better UX when interacting with the H and S sliders. const colorValue = isInternalColorSameAsReceivedColor ? internalHSLA - : colorPropHSL; + : colorPropHSLA; const updateHSLAValue = ( - partialNewValue: Partial< typeof colorPropHSL > + partialNewValue: Partial< typeof colorPropHSLA > ) => { // Update internal HSLA color. setInternalHSLA( ( prevValue ) => ( { From 3c6a86093c93b4c67b740d0d833942784189b64d Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 10 Jan 2024 16:53:43 +0100 Subject: [PATCH 5/7] Add unit test --- .../src/color-picker/test/index.tsx | 141 +++++++++++++++--- 1 file changed, 124 insertions(+), 17 deletions(-) diff --git a/packages/components/src/color-picker/test/index.tsx b/packages/components/src/color-picker/test/index.tsx index 8d584d626487a..026777523d434 100644 --- a/packages/components/src/color-picker/test/index.tsx +++ b/packages/components/src/color-picker/test/index.tsx @@ -1,9 +1,14 @@ /** * External dependencies */ -import { screen, render } from '@testing-library/react'; +import { fireEvent, screen, render, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +/** + * WordPress dependencies + */ +import { useState } from '@wordpress/element'; + /** * Internal dependencies */ @@ -133,20 +138,34 @@ describe( 'ColorPicker', () => { } ); } ); - describe.each( [ - [ 'hue', 'Hue', '#aad52a' ], - [ 'saturation', 'Saturation', '#20dfdf' ], - [ 'lightness', 'Lightness', '#95eaea' ], - ] )( 'HSL inputs', ( colorInput, inputLabel, expected ) => { - it( `should fire onChange with the correct value when the ${ colorInput } value is updated`, async () => { + describe( 'HSL inputs', () => { + it( 'sliders', async () => { const user = userEvent.setup(); const onChange = jest.fn(); - const color = '#2ad5d5'; + + const ControlledColorPicker = ( { + onChange: onChangeProp, + ...restProps + }: React.ComponentProps< typeof ColorPicker > ) => { + const [ colorState, setColorState ] = useState( '#000000' ); + + const internalOnChange: typeof onChangeProp = ( newColor ) => { + onChangeProp?.( newColor ); + setColorState( newColor ); + }; + + return ( + + ); + }; render( - ); @@ -156,16 +175,104 @@ describe( 'ColorPicker', () => { await user.selectOptions( formatSelector, 'hsl' ); - const inputElement = screen.getByRole( 'spinbutton', { - name: inputLabel, + const hueSliders = screen.getAllByRole( 'slider', { + name: 'Hue', } ); - expect( inputElement ).toBeVisible(); + expect( hueSliders ).toHaveLength( 2 ); - await user.clear( inputElement ); - await user.type( inputElement, '75' ); + // Reason for the `!` post-fix expression operator: if the check above + // doesn't fail, we're guaranteed that `hueSlider` is not undefined. + const hueSlider = hueSliders.at( -1 )!; + const saturationSlider = screen.getByRole( 'slider', { + name: 'Saturation', + } ); + const lightnessSlider = screen.getByRole( 'slider', { + name: 'Lightness', + } ); + const hueNumberInput = screen.getByRole( 'spinbutton', { + name: 'Hue', + } ); + const saturationNumberInput = screen.getByRole( 'spinbutton', { + name: 'Saturation', + } ); + const lightnessNumberInput = screen.getByRole( 'spinbutton', { + name: 'Lightness', + } ); - expect( onChange ).toHaveBeenCalledTimes( 3 ); - expect( onChange ).toHaveBeenLastCalledWith( expected ); + // All initial inputs should have a value of `0` since the color is black. + expect( hueSlider ).toHaveValue( '0' ); + expect( saturationSlider ).toHaveValue( '0' ); + expect( lightnessSlider ).toHaveValue( '0' ); + expect( hueNumberInput ).toHaveValue( 0 ); + expect( saturationNumberInput ).toHaveValue( 0 ); + expect( lightnessNumberInput ).toHaveValue( 0 ); + + // Interact with the Hue slider, it should change its value (and the + // value of the associated number input), but it shouldn't cause the + // `onChange` callback to fire, since the resulting color is still black. + fireEvent.change( hueSlider, { target: { value: 80 } } ); + + expect( hueSlider ).toHaveValue( '80' ); + expect( hueNumberInput ).toHaveValue( 80 ); + expect( onChange ).not.toHaveBeenCalled(); + + // Interact with the Saturation slider, it should change its value (and the + // value of the associated number input), but it shouldn't cause the + // `onChange` callback to fire, since the resulting color is still black. + fireEvent.change( saturationSlider, { target: { value: 50 } } ); + + expect( saturationSlider ).toHaveValue( '50' ); + expect( saturationNumberInput ).toHaveValue( 50 ); + expect( onChange ).not.toHaveBeenCalled(); + + // Interact with the Lightness slider, it should change its value (and the + // value of the associated number input). It should also cause the + // `onChange` callback to fire, since changing the lightness actually + // causes the color to change. + fireEvent.change( lightnessSlider, { target: { value: 30 } } ); + + await waitFor( () => + expect( lightnessSlider ).toHaveValue( '30' ) + ); + expect( lightnessNumberInput ).toHaveValue( 30 ); + expect( onChange ).toHaveBeenCalledTimes( 1 ); + expect( onChange ).toHaveBeenLastCalledWith( '#597326' ); + } ); + + describe.each( [ + [ 'hue', 'Hue', '#aad52a' ], + [ 'saturation', 'Saturation', '#20dfdf' ], + [ 'lightness', 'Lightness', '#95eaea' ], + ] )( 'HSL inputs', ( colorInput, inputLabel, expected ) => { + it( `should fire onChange with the correct value when the ${ colorInput } value is updated`, async () => { + const user = userEvent.setup(); + const onChange = jest.fn(); + const color = '#2ad5d5'; + + render( + + ); + + const formatSelector = screen.getByRole( 'combobox' ); + expect( formatSelector ).toBeVisible(); + + await user.selectOptions( formatSelector, 'hsl' ); + + const inputElement = screen.getByRole( 'spinbutton', { + name: inputLabel, + } ); + expect( inputElement ).toBeVisible(); + + await user.clear( inputElement ); + await user.type( inputElement, '75' ); + + expect( onChange ).toHaveBeenCalledTimes( 3 ); + expect( onChange ).toHaveBeenLastCalledWith( expected ); + } ); } ); } ); } ); From cb3303d9da2fc89533eb4edaeccd72e3b3445610 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 10 Jan 2024 19:11:00 +0100 Subject: [PATCH 6/7] Set internal SLA only when not calling onChange to reduce visual glitches --- packages/components/src/color-picker/hsl-input.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/components/src/color-picker/hsl-input.tsx b/packages/components/src/color-picker/hsl-input.tsx index 118677becde4e..8d2b0c7c44489 100644 --- a/packages/components/src/color-picker/hsl-input.tsx +++ b/packages/components/src/color-picker/hsl-input.tsx @@ -41,12 +41,6 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { const updateHSLAValue = ( partialNewValue: Partial< typeof colorPropHSLA > ) => { - // Update internal HSLA color. - setInternalHSLA( ( prevValue ) => ( { - ...prevValue, - ...partialNewValue, - } ) ); - const nextOnChangeValue = colord( { ...colorValue, ...partialNewValue, @@ -54,8 +48,14 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => { // Fire `onChange` only if the resulting color is different from the // current one. + // Otherwise, update the internal HSLA color to cause a re-render. if ( ! color.isEqual( nextOnChangeValue ) ) { onChange( nextOnChangeValue ); + } else { + setInternalHSLA( ( prevHSLA ) => ( { + ...prevHSLA, + ...partialNewValue, + } ) ); } }; From 6577756aed006b10c34be950e193b82f7a920806 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 10 Jan 2024 19:27:14 +0100 Subject: [PATCH 7/7] More test --- .../src/color-picker/test/index.tsx | 79 +++++++++++++++++-- 1 file changed, 73 insertions(+), 6 deletions(-) diff --git a/packages/components/src/color-picker/test/index.tsx b/packages/components/src/color-picker/test/index.tsx index 026777523d434..98e059d5994de 100644 --- a/packages/components/src/color-picker/test/index.tsx +++ b/packages/components/src/color-picker/test/index.tsx @@ -13,6 +13,7 @@ import { useState } from '@wordpress/element'; * Internal dependencies */ import { ColorPicker } from '..'; +import { click } from '@ariakit/test'; const hslaMatcher = expect.objectContaining( { h: expect.any( Number ), @@ -139,7 +140,7 @@ describe( 'ColorPicker', () => { } ); describe( 'HSL inputs', () => { - it( 'sliders', async () => { + it( 'sliders should use accurate H and S values based on user interaction when possible', async () => { const user = userEvent.setup(); const onChange = jest.fn(); @@ -155,11 +156,16 @@ describe( 'ColorPicker', () => { }; return ( - + <> + + + ); }; @@ -237,6 +243,67 @@ describe( 'ColorPicker', () => { expect( lightnessNumberInput ).toHaveValue( 30 ); expect( onChange ).toHaveBeenCalledTimes( 1 ); expect( onChange ).toHaveBeenLastCalledWith( '#597326' ); + + // Interact with the Lightness slider, setting to 100 (ie. white). + // It should also cause the `onChange` callback to fire, and reset the + // hue and saturation inputs to `0`. + fireEvent.change( lightnessSlider, { target: { value: 100 } } ); + + await waitFor( () => + expect( lightnessSlider ).toHaveValue( '100' ) + ); + expect( lightnessNumberInput ).toHaveValue( 100 ); + expect( hueSlider ).toHaveValue( '0' ); + expect( saturationSlider ).toHaveValue( '0' ); + expect( hueNumberInput ).toHaveValue( 0 ); + expect( saturationNumberInput ).toHaveValue( 0 ); + expect( onChange ).toHaveBeenCalledTimes( 2 ); + expect( onChange ).toHaveBeenLastCalledWith( '#ffffff' ); + + // Interact with the Hue slider, it should change its value (and the + // value of the associated number input), but it shouldn't cause the + // `onChange` callback to fire, since the resulting color is still white. + fireEvent.change( hueSlider, { target: { value: 147 } } ); + + expect( hueSlider ).toHaveValue( '147' ); + expect( hueNumberInput ).toHaveValue( 147 ); + expect( onChange ).toHaveBeenCalledTimes( 2 ); + + // Interact with the Saturation slider, it should change its value (and the + // value of the associated number input), but it shouldn't cause the + // `onChange` callback to fire, since the resulting color is still white. + fireEvent.change( saturationSlider, { target: { value: 82 } } ); + + expect( saturationSlider ).toHaveValue( '82' ); + expect( saturationNumberInput ).toHaveValue( 82 ); + expect( onChange ).toHaveBeenCalledTimes( 2 ); + + // Interact with the Lightness slider, it should change its value (and the + // value of the associated number input). It should also cause the + // `onChange` callback to fire, since changing the lightness actually + // causes the color to change. + fireEvent.change( lightnessSlider, { target: { value: 14 } } ); + + await waitFor( () => + expect( lightnessSlider ).toHaveValue( '14' ) + ); + expect( lightnessNumberInput ).toHaveValue( 14 ); + expect( onChange ).toHaveBeenCalledTimes( 3 ); + expect( onChange ).toHaveBeenLastCalledWith( '#064121' ); + + // Set the color externally. All inputs should update to match the H/S/L + // value of the new color. + const setColorButton = screen.getByRole( 'button', { + name: /set color/i, + } ); + await click( setColorButton ); + + expect( hueSlider ).toHaveValue( '208' ); + expect( hueNumberInput ).toHaveValue( 208 ); + expect( saturationSlider ).toHaveValue( '44' ); + expect( saturationNumberInput ).toHaveValue( 44 ); + expect( lightnessSlider ).toHaveValue( '52' ); + expect( lightnessNumberInput ).toHaveValue( 52 ); } ); describe.each( [