Refactor: ColorPicker to use hooks

[daddou-ma]

Related to #22890

Description

Refactored ColorPicker Components to functional components.

How has this been tested?

I tested the component from the storybook, npm run storybook:dev

Types of changes

Refactor : ColorPicker to functional component.
Refactor : ColorPicker Hue to functional component.
Refactor : ColorPicker Saturation to functional component.
Refactor : ColorPicker Alpha to functional component.
Refactor : ColorPicker Inputs to functional component.

Update Test : ColorPicker Tests by putting test into TestRenderer.act() function.
Update Snapshot: ColorPalette Snapshot by removing Pure and withInstance HOCs.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[ellatrix]

We should only clean up if mouseDown is true. I would suggest to return early if mouseDown is false.

if ( ! mouseDown ) {
  return;
}

ownerDocument.addEventListener( 'mousemove', handleChange );

return () => {
  ownerDocument.removeEventListener( 'mousemove', handleChange );
}

In this case we're returning the function, so it's ok for it to be anonymous. The clean up function is anonymous everywhere in the codebase.

[ellatrix]

So, regarding the named functions, I only do it if you'd otherwise assign it to a constant. E.g. const onChange = () => ....

[daddou-ma]

I agree it's much better like this. i made the changes in daddou-ma/gutenberg@2bbe328

[ellatrix]

Tbh, I'm not sure how this works... If onChangeComplete is a new function on every render, the debouncing should fail?

[ellatrix]

At least it should fail between re-renders. Not sure how many times this component re-renders.

[daddou-ma]

I think i should use a useCallback hook. Right ?!

[ellatrix]

Yes, it would avoid creating new functions on every render, which is good for debounce and throttle. The only problem is that it should be done by the consumer of ColorPicker, since only the consumer knows if there are any dependencies.

[ellatrix]

Maybe it's ok to leave it as it is now and only add useCallback for internal functions over which ColorPicker has control.

[daddou-ma]

a added useCallback in 716869d6cb2724ce2f136be8fec380cd409ad825 inside the ColorPicker.

[ellatrix]

This is useless if parent components return new functions on every render though: https://github.com/WordPress/gutenberg/search?q=onChangeComplete.

Not sure what's the best way around it.

[daddou-ma]

@ellatrix should we add the new useThrottle as a feature in CHANGELOG.md of compose package ?

[ellatrix]

commitValues is a new function on every render which means the useCallback won't work.

[daddou-ma]

What if i move useThrottle from Saturation to ColorPicker ? by putting commitValues function inside useThrottle and useCallback hooks in ColorPicker level, so the throttled callback will be passed to Saturation.

[ellatrix]

I'm not sure if it will matter much. As long as there is a dependency on onChangeComplete, useCallback will just return a new function on every call too. This may be alright if the function is not re-rendering all the time. I'll see if I can test this PR tomorrow.

[daddou-ma]

Thank you @ellatrix, i've tested with a higher delay in useDebounce and useThrottle and I they are working well for me. and re-render of ColorPicker happen on every UI interraction.

[ellatrix]

Sorry for the delay in getting back to you. Could you rebase the PR?

[daddou-ma]

Sorry for the delay in getting back to you. Could you rebase the PR?

sorry too for taking long. I rebased the branch.

[daddou-ma]

Sorry for the delay in getting back to you. Could you rebase the PR?

@ellatrix I've rebased the PR into 'trunk'.

[stokesman]

I haven't gone over every single line of the changes but have left ideas for addressing the concerns previously raised.

To support my suggestion that the Hue and Alpha components should throttle their calls to onChange as does Saturation, see this video recorded from a recent build from trunk. The Hue component gets quite chunky in its dragging updates and can even cause drifted execution after draging ends. Sure, it's out of scope of a pure refactor but is a small ask.

hue-slider-laggage.mp4

[stokesman]

These are stable across rerenders. To maintain that in the refactor I think the easiest option is to put them all in a lazily initialized ref. As an example of that, outside the component function something like this:

function createColorOperations( setColors ) {
	const commitValues = ( data ) => { /* … */ };
	const resetDraftValues = () => { /* … */ };
	const setDraftValues = ( data ) => { /* … */ };
	const handleInputChange = ( data ) => { /* … */ };
	const throttledCommitValues = useThrottle( commitValues, 50 );
	return { 
		commitValues,
		resetDraftValues,
		setDraftValues,
		handleInputChange,
		throttledCommitValues
	};
}

Then use it inside the component function like so:

	const colorOperations = useRef();
	if ( ! colorOperations.current ) {
		colorOperations.current = createColorOperations( setColors );
	}
	const {
		commitValues,
		resetDraftValues,
		setDraftValues,
		handleInputChange,
		throttledCommitValues
	} = colorOperations.current;

That will make commitValues stable so it can then be throttled as done by the Saturation component. On a more general note, Hue and Alpha should probably also be throttling their use of it and that's why I included creating the throttled version of the function above (a bit like @daddou-ma had suggested https://github.com/WordPress/gutenberg/pull/26770/files#r521300047).

From there I think we could remove the debounce of onChangeComplete which was apparently only ever an intent. As Ella suspected https://github.com/WordPress/gutenberg/pull/26770/files#r519931211, it does not work (I've tested from trunk to verify). Beyond that, debounce doesn't seem right for this and throttle seems more appropriate.

[stokesman]

Suggested change

debouncedOnChangeComplete( newColors );

The current implementation passes this as a callback for .setState so it's asynchronous. To match that in the refactor this could be called via an effect hook (useUpdateEffect from '../utils' to avoid the initial render). This would simplify creating a stable commitValues as in my preceding comment and is the main reason I'm suggesting it.

[Mamaduka]

Thanks for contributing, @daddou-ma!

The ColorPicker component got an update in #33714. It now uses functional components and hooks.