Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix unexpected movements, and closing on first click in the color picker #35670

Merged
merged 1 commit into from
Oct 25, 2021
+12 −36
Merged

Fix unexpected movements, and closing on first click in the color picker #35670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions packages/components/src/color-picker/component.tsx
View file
Open in desktop
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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the README and any potential JSDocs types to add the Default value of the defaultValue prop — @jorgefilipecosta would you be able to follow up with this small change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will work on a PR with this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jorgefilipecosta , have you had any time yet to work on a PR with this change ? Thank you!

copyFormat,
...divProps
} = useContextSystem( props, 'ColorPicker' );

const [ color, setColor ] = useControlledValue( {
onChange,
value: colorProp,
defaultValue,
} );
Comment on lines -69 to -73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if moving away from useControlledValue caused any regressions — @diegohaz could you take a quick look?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm! I don't understand why we need to move away from useControlledValue in this case, but maybe this is an issue with the useControlledValue function itself.

I wonder if the fact that we're not memoizing the state updater here is causing any problem since the original updater from useState is supposed to be stable:

gutenberg/packages/components/src/utils/hooks/use-controlled-value.ts

Lines 35 to 38 in 8ccd5e1

setValue = ( nextValue ) => {
onChange( nextValue );
setState( nextValue );
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I removed the useControlledValue call was that everything seemed to be working as before without it. It seems on this case it is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just cc'ing @sarayourfriend who wrote this part of the ColorPicker component, just to make sure that removing useControlledValue was ok in this scenario

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind using useControlledValue was to allow the color picker to behave like a standard React input, that is, with or without a controlled external value.

<ColorPicker onChange={setColor} />

Should work even though the color prop isn't being passed. Just like <input onChange={setValue} /> works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, thank you Sara!

@jorgefilipecosta , would it possible to add the useControlledValue hook back to the component?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jorgefilipecosta , just wondering if you've had any time to work on this, or if you plan to anytime soon? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ciampo and @jorgefilipecosta, I was investigating an issue with the color picker and gradients (reported in #36899) and it looks like re-instating useControlledValue and the debounce call resolves the issue, so I've opened up a PR in #36941 if either or you get the chance to take a look. I'm not very familiar with the ColorPicker component, so it can probably use some 👀 from folks who've worked with it before 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @andrewserong , I'll definitely have a look


// 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
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand why we switched from an object-based to a string-based picker, I don't understand why we switched also from an HSL-based to an RBG-based picker — could you explain why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format we use outside the component is hex code which maps directly to RGB just by converting hex to decimal. Since we are changing to a string-based picker why not use the RGB one when it maps more directly? What advantage do we get in using an HSL string-based picker?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it looks like the only difference would be about the input string format — while the underlying ColorPicker would still be the same. I guess using a HSL picker made sense previously, when this component used HSL internally to represent color values — but with the current state of the component, this change should be fine :)

Thank you for explaining!

? 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
View file
Open in desktop
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