Skip to content

Commit

Permalink
Fix unexpected movements, and closing on first click in the color pic…
Browse files Browse the repository at this point in the history
…ker (#35670)
  • Loading branch information
jorgefilipecosta authored and vcanales committed Oct 27, 2021
1 parent e5d9d72 commit c6bb3ce
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 36 deletions.
23 changes: 6 additions & 17 deletions packages/components/src/color-picker/component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import namesPlugin from 'colord/plugins/names';
*/
import { useState, useMemo } from '@wordpress/element';
import { settings } from '@wordpress/icons';
import { useDebounce } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

/**
Expand All @@ -33,7 +32,6 @@ import {
import { ColorDisplay } from './color-display';
import { ColorInput } from './color-input';
import { Picker } from './picker';
import { useControlledValue } from '../utils/hooks';

import type { ColorType } from './types';

Expand All @@ -59,32 +57,23 @@ const ColorPicker = (
) => {
const {
enableAlpha = false,
color: colorProp,
color,
onChange,
defaultValue,
defaultValue = '#fff',
copyFormat,
...divProps
} = useContextSystem( props, 'ColorPicker' );

const [ color, setColor ] = useControlledValue( {
onChange,
value: colorProp,
defaultValue,
} );

// Use a safe default value for the color and remove the possibility of `undefined`.
const safeColordColor = useMemo( () => {
return color ? colord( color ) : colord( '#fff' );
}, [ color ] );

// Debounce to prevent rapid changes from conflicting with one another.
const debouncedSetColor = useDebounce( setColor );
return color ? colord( color ) : colord( defaultValue );
}, [ color, defaultValue ] );

const handleChange = useCallback(
( nextValue: Colord ) => {
debouncedSetColor( nextValue.toHex() );
onChange( nextValue.toHex() );
},
[ debouncedSetColor ]
[ onChange ]
);

const [ showInputs, setShowInputs ] = useState< boolean >( false );
Expand Down
10 changes: 6 additions & 4 deletions packages/components/src/color-picker/picker.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { HslColorPicker, HslaColorPicker } from 'react-colorful';
import { RgbStringColorPicker, RgbaStringColorPicker } from 'react-colorful';
import { colord, Colord } from 'colord';

/**
Expand All @@ -15,12 +15,14 @@ interface PickerProps {
}

export const Picker = ( { color, enableAlpha, onChange }: PickerProps ) => {
const Component = enableAlpha ? HslaColorPicker : HslColorPicker;
const hslColor = useMemo( () => color.toHsl(), [ color ] );
const Component = enableAlpha
? RgbaStringColorPicker
: RgbStringColorPicker;
const rgbColor = useMemo( () => color.toRgbString(), [ color ] );

return (
<Component
color={ hslColor }
color={ rgbColor }
onChange={ ( nextColor ) => {
onChange( colord( nextColor ) );
} }
Expand Down
15 changes: 0 additions & 15 deletions packages/components/src/color-picker/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ function moveReactColorfulSlider( sliderElement, from, to ) {
fireEvent( sliderElement, new FakeMouseEvent( 'mousemove', to ) );
}

const sleep = ( ms ) => {
const promise = new Promise( ( resolve ) => setTimeout( resolve, ms ) );
jest.advanceTimersByTime( ms + 1 );
return promise;
};

const hslaMatcher = expect.objectContaining( {
h: expect.any( Number ),
s: expect.any( Number ),
Expand Down Expand Up @@ -93,9 +87,6 @@ describe( 'ColorPicker', () => {
{ pageX: 10, pageY: 10 }
);

// `onChange` is debounced so we need to sleep for at least 1ms before checking that onChange was called
await sleep( 1 );

expect( onChangeComplete ).toHaveBeenCalledWith(
legacyColorMatcher
);
Expand All @@ -117,9 +108,6 @@ describe( 'ColorPicker', () => {
{ pageX: 10, pageY: 10 }
);

// `onChange` is debounced so we need to sleep for at least 1ms before checking that onChange was called
await sleep( 1 );

expect( onChange ).toHaveBeenCalledWith(
expect.stringMatching( /^#([a-fA-F0-9]{8})$/ )
);
Expand Down Expand Up @@ -150,9 +138,6 @@ describe( 'ColorPicker', () => {
{ pageX: 10, pageY: 10 }
);

// `onChange` is debounced so we need to sleep for at least 1ms before checking that onChange was called
await sleep( 1 );

expect( onChange ).toHaveBeenCalledWith(
expect.stringMatching( /^#([a-fA-F0-9]{6})$/ )
);
Expand Down

0 comments on commit c6bb3ce

Please sign in to comment.