diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index c7c1a515a64cec..0b5feb1203b985 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)). diff --git a/packages/components/src/color-picker/hsl-input.tsx b/packages/components/src/color-picker/hsl-input.tsx index 3331a97b3d4de6..8d2b0c7c444899 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, useMemo } 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 colorPropHSLA = useMemo( () => color.toHsl(), [ color ] ); + + const [ internalHSLA, setInternalHSLA ] = useState( { ...colorPropHSLA } ); + + const isInternalColorSameAsReceivedColor = color.isEqual( + colord( internalHSLA ) + ); + + useEffect( () => { + if ( ! isInternalColorSameAsReceivedColor ) { + // Keep internal HSLA color up to date with the received color prop + setInternalHSLA( colorPropHSLA ); + } + }, [ 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, + // 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 + : colorPropHSLA; + + const updateHSLAValue = ( + partialNewValue: Partial< typeof colorPropHSLA > + ) => { + const nextOnChangeValue = colord( { + ...colorValue, + ...partialNewValue, + } ); + + // 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, + } ) ); + } + }; 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 } ); } } /> ) } diff --git a/packages/components/src/color-picker/test/index.tsx b/packages/components/src/color-picker/test/index.tsx index 8d584d626487a4..98e059d5994ded 100644 --- a/packages/components/src/color-picker/test/index.tsx +++ b/packages/components/src/color-picker/test/index.tsx @@ -1,13 +1,19 @@ /** * 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 */ import { ColorPicker } from '..'; +import { click } from '@ariakit/test'; const hslaMatcher = expect.objectContaining( { h: expect.any( Number ), @@ -133,20 +139,39 @@ 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 should use accurate H and S values based on user interaction when possible', 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 +181,165 @@ 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', + } ); + + // 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' ); + + // 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( expected ); + 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( [ + [ '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 ); + } ); } ); } ); } );