useColors: directly pass ref for color detecting #19474

ellatrix · 2020-01-07T15:06:06Z

Description

This PR removes the <ColorDetector /> component in favour of passing a reference of the target element directly to useColors.

Removes an additional div element that is rendered for each paragraph.
Avoids needing to query the DOM for the target node.
Avoids the use of withFallbackStyles.
I also swapped out the detected color references for state, so the warnings are displayed immediately instead of delayed to the next rerender.

How has this been tested?

Trigger the contrast warning on the paragraph and heading block by selecting a text or background color together with inherited colours (detected).

Screenshots

Types of changes

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 · 2020-01-07T15:07:07Z

packages/block-library/src/heading/edit.js

@@ -56,8 +59,8 @@ function HeadingEdit( {
 			</InspectorControls>
 			{ InspectorControlsColorPanel }
 			<TextColor>
-				<ColorDetector querySelector='[contenteditable="true"]' />


This component should no longer be needed as the colours can be calculated in an effect.

packages/block-editor/src/components/colors/use-colors.js

epiqueras

What's the best way to test the color detecting feature?

The contrast checker warnings for blocks with inherited colors.

packages/block-editor/src/components/colors/use-colors.js

epiqueras · 2020-01-07T15:16:54Z

packages/block-editor/src/components/colors/use-colors.js

@@ -219,26 +221,10 @@ export default function __experimentalUseColors(
 		return (
 			( needsBackgroundColor || needsColor ) &&
 			withFallbackStyles(


This won't work, withFallbackStyles returns a component and calls the passed-in function with its root node's ref upon mounting.

Now we can query the new target ref here.

I know, I was still figuring out that part. Rewrote it now. Looks like we need to use state to set the colours so the component rerenders?

That's what this dep is for:

https://github.com/WordPress/gutenberg/blob/a7860eecc11878403b33ccf2dcfa16672179bad6/packages/block-editor/src/components/colors/use-colors.js#L248-L254

I meant for <ColorPanel> to rerender. How does it know when detected colours changed if it's a ref?

The detected color can't change without you doing something that updates the component and, therefore, the color panel.

There were some edge cases, but we decided they were reasonable.

ellatrix · 2020-01-07T15:23:42Z

packages/block-editor/src/components/colors/use-colors.js

@@ -216,48 +217,27 @@ export default function __experimentalUseColors(
 				break;
 			}
 		}
-		return (
-			( needsBackgroundColor || needsColor ) &&
-			withFallbackStyles(


I don't think withFallbackStyles is need anymore since we have direct access to the element.

ellatrix · 2020-01-07T15:39:49Z

@epiqueras @jorgefilipecosta I'm not sure if this correctly works in master. When I select a light colour (with light computed background), there's no contrast warning. When I select a black background colour (with a dark computed text colour), the contrast warning only shows on the next block update (e.g. typing some text).

epiqueras · 2020-01-07T15:49:14Z

I'm not sure if this correctly works in master.

Something has regressed.

epiqueras · 2020-01-07T16:02:07Z

@ellatrix Found it!

https://github.com/WordPress/gutenberg/pull/19046/files

backgroundColorNode.parentNode === Node.ELEMENT_NODE is always evaluating to false.

cc @jorgefilipecosta

ellatrix · 2020-01-08T09:56:36Z

@epiqueras Fixed in in #19500. This PR is now ready. I also swapped out the references for state so the warnings are displayed instantly instead of being delayed to a next rerender.

epiqueras

We need the following option instead of just targetRef:

colorDetector: {
	targetRef,
	backgroundColorTargetRef = targetRef,
	textColorTargetRef = targetRef,
} = {},

There are cases where the text won't be a child of the element setting a background color.

ellatrix · 2020-01-08T14:32:16Z

There are cases where the text won't be a child of the element setting a background color.

Could you point me to these cases? I only see paragraph and heading using this hook. Or do you have use cases outside core?

epiqueras · 2020-01-08T14:36:02Z

The goal is to refactor all blocks with colors to use this hook.

#18547 (comment)

ellatrix · 2020-01-08T14:54:05Z

@epiqueras Done in 606137a.

epiqueras · 2020-01-08T14:59:45Z

targetRef is not descriptive enough, hence why I suggested nesting the 3 new options under colorDetector.

ellatrix · 2020-01-08T15:04:38Z

@epiqueras Makes sense. Done now.

packages/block-editor/src/components/colors/use-colors.js

ellatrix · 2020-01-08T17:24:55Z

Thanks for the review!

epiqueras · 2020-01-08T17:27:36Z

Thanks for the fixes!

ellatrix requested review from ajitbohra, daniloercoli, etoledom, jorgefilipecosta, SergioEstevao, Soean, talldan and youknowriad as code owners January 7, 2020 15:06

ellatrix commented Jan 7, 2020

View reviewed changes

packages/block-editor/src/components/colors/use-colors.js Outdated Show resolved Hide resolved

epiqueras requested changes Jan 7, 2020

View reviewed changes

ellatrix commented Jan 7, 2020

View reviewed changes

ellatrix requested a review from epiqueras January 7, 2020 15:50

ellatrix added the [Type] Code Quality Issues or PRs that relate to code quality label Jan 7, 2020

ellatrix mentioned this pull request Jan 8, 2020

useColors: fix contrast check #19500

Merged

6 tasks

useColors: directly pass ref for color detecting

e2e9a34

ellatrix added the [Type] Performance Related to performance efforts label Jan 8, 2020

ellatrix added 3 commits January 8, 2020 10:39

Remove withFallbackStyles

ca82326

Fix useColors parameters

4279ff9

Use state for detected colors so warning is immediately displayed

d764558

ellatrix force-pushed the try/use-colors-ref branch from dbd22a5 to d764558 Compare January 8, 2020 09:53

epiqueras requested changes Jan 8, 2020

View reviewed changes

Restore separate targets

606137a

ellatrix requested a review from epiqueras January 8, 2020 14:54

Address feedback

c78cd83

epiqueras approved these changes Jan 8, 2020

View reviewed changes

packages/block-editor/src/components/colors/use-colors.js Show resolved Hide resolved

ellatrix merged commit 7c2b50c into master Jan 8, 2020

ellatrix deleted the try/use-colors-ref branch January 8, 2020 17:23

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

useColors: directly pass ref for color detecting #19474

useColors: directly pass ref for color detecting #19474

ellatrix commented Jan 7, 2020 •

edited

Loading

ellatrix Jan 7, 2020

epiqueras left a comment

epiqueras Jan 7, 2020

ellatrix Jan 7, 2020

epiqueras Jan 7, 2020

ellatrix Jan 7, 2020 •

edited

Loading

epiqueras Jan 7, 2020

ellatrix Jan 7, 2020

ellatrix commented Jan 7, 2020

epiqueras commented Jan 7, 2020

epiqueras commented Jan 7, 2020

ellatrix commented Jan 8, 2020

epiqueras left a comment

ellatrix commented Jan 8, 2020

epiqueras commented Jan 8, 2020

ellatrix commented Jan 8, 2020

epiqueras commented Jan 8, 2020

ellatrix commented Jan 8, 2020

ellatrix commented Jan 8, 2020

epiqueras commented Jan 8, 2020

useColors: directly pass ref for color detecting #19474

useColors: directly pass ref for color detecting #19474

Conversation

ellatrix commented Jan 7, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix Jan 7, 2020

Choose a reason for hiding this comment

epiqueras left a comment

Choose a reason for hiding this comment

epiqueras Jan 7, 2020

Choose a reason for hiding this comment

ellatrix Jan 7, 2020

Choose a reason for hiding this comment

epiqueras Jan 7, 2020

Choose a reason for hiding this comment

ellatrix Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

epiqueras Jan 7, 2020

Choose a reason for hiding this comment

ellatrix Jan 7, 2020

Choose a reason for hiding this comment

ellatrix commented Jan 7, 2020

epiqueras commented Jan 7, 2020

epiqueras commented Jan 7, 2020

ellatrix commented Jan 8, 2020

epiqueras left a comment

Choose a reason for hiding this comment

ellatrix commented Jan 8, 2020

epiqueras commented Jan 8, 2020

ellatrix commented Jan 8, 2020

epiqueras commented Jan 8, 2020

ellatrix commented Jan 8, 2020

ellatrix commented Jan 8, 2020

epiqueras commented Jan 8, 2020

ellatrix commented Jan 7, 2020 •

edited

Loading

ellatrix Jan 7, 2020 •

edited

Loading