Use Colors: Add text color detection support. by epiqueras · Pull Request #18547 · WordPress/gutenberg

epiqueras · 2019-11-15T17:07:05Z

Description

This PR adds support for detecting text colors using the true option introduced for useColors contrast checking by #18237.

How has this been tested?

All uses of useColors were verified to continue to work as expected.

Types of Changes

New Feature: useColors contrast checking now supports text color detection.

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. .

jorgefilipecosta · 2019-11-15T17:11:59Z

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

 			{ InspectorControlsColorPanel }
 			<TextColor>
-				<BackgroundColorDetector querySelector='[contenteditable="true"]' />
+				<ColorDetector querySelector='[contenteditable="true"]' />


In the button block, the background and color elements are not the same. Would it make sense to have a colorSelector and backgroundSelector? To allow the detector to work when background and color use different elements?

Yes, I was just about to ask you that 😄

youknowriad · 2019-11-15T17:18:59Z

Drive by comment unrelated with the current PR.

Can we add a README for this hook, I feel it will make it way easier for folks to understand and even contribute to the hook. It's also very valuable when we'll open this hook for third-party block authors;

epiqueras · 2019-11-15T17:21:43Z

Definitely. I also want to try making an interactive MDX guide for it in the storybook, under "Block Utilities".

epiqueras · 2019-11-20T22:52:18Z

@jorgefilipecosta This should be ready to go now 😄

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

jorgefilipecosta

I rebased the useColors on paragraph branch #18148 above this branch as a way to test this PR and I noticed some issues that I left as comments.

jorgefilipecosta · 2019-11-25T15:08:25Z

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

+	const { TextColor, InspectorControlsColorPanel, ColorDetector } = __experimentalUseColors(
 		[ { name: 'textColor', property: 'color' } ],
 		{
 			contrastCheckers: { backgroundColor: true },


Should we also add textColor: true?

That would show the warning when the inherited text color does not have enough contrast.

Is that something we want? Considering this block does not control background color, an inherited text color without enough contrast would also trigger a warning in the parent level and should probably be fixed there.

In the paragraph, we need color detection on the text and background color. I'm not sure if we should show a warning if no color was selected at all. Maybe we should have a condition to not show a warning if the user did not use a background or text color?

Then it becomes kind of confusing.

Why do I see a warning sometimes, and sometimes I don't?

I think we should just always show the warning now. So I should add textColor: true, what do you think?

Yes I guess we can add it 👍

Done 😄 9ae645f

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

jorgefilipecosta · 2019-11-25T16:02:50Z

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

+		}
+		return (
+			( needsBackgroundColor || needsColor ) &&
+			withFallbackStyles(


We need to re-execute the color detection when a color changes. On the paragraph block, we want to automatically detect background and text color contrastCheckers: { backgroundColor: true, textColor: true, fontSize: fontSize.size },, we can also change both colors. The contrast warnings don't update when a color challenges but if we go to the code editor and switch back the visual editor to force the component to remount the warnings appear as expected.

This seems to be working after your last rebase?

No, I think the warnings still don't appear when expected:

We can see the warning appears when it should not, and disappears when it should appear. If I type something and the warning should appear, it appears after I type. Not sure why typing interferes with this, but we should make sure the color computation does not happen on each character typed.

This was happening because setting a ref does not re-render the component, so the contrast checker used a stale value until the component had any sort of update.

Fixed: 45c81d4.

jorgefilipecosta

I verified the problems I noticed were solved, nice work @epiqueras 👍

epiqueras · 2019-12-02T16:06:12Z

Thanks for all the thorough reviews 😄

epiqueras added [Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience labels Nov 15, 2019

epiqueras added this to the Future milestone Nov 15, 2019

epiqueras requested a review from jorgefilipecosta November 15, 2019 17:07

epiqueras requested review from Soean, ajitbohra, ellatrix, talldan and youknowriad as code owners November 15, 2019 17:07

epiqueras self-assigned this Nov 15, 2019

jorgefilipecosta reviewed Nov 15, 2019

View reviewed changes

youknowriad reviewed Nov 21, 2019

View reviewed changes

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

epiqueras added 2 commits November 25, 2019 14:44

Use Colors: Add text color detection support.

465ce64

Use Colors: Support specific selectors for color detection.

a537bc0

jorgefilipecosta force-pushed the add/text-color-detection-to-use-colors-hook branch from 53fb172 to a537bc0 Compare November 25, 2019 14:45

jorgefilipecosta reviewed Nov 25, 2019

View reviewed changes

epiqueras mentioned this pull request Nov 26, 2019

Use colors hook in paragraph #18148

Merged

epiqueras added 2 commits November 26, 2019 12:59

Use Colors: Pass down refs to contrast checker to avoid stale values.

45c81d4

Heading: Detect inherited text color for contrast checking.

9ae645f

jorgefilipecosta approved these changes Dec 2, 2019

View reviewed changes

epiqueras merged commit 2fd13b6 into master Dec 2, 2019

youknowriad deleted the add/text-color-detection-to-use-colors-hook branch December 3, 2019 08:52

youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019

epiqueras mentioned this pull request Jan 8, 2020

useColors: directly pass ref for color detecting #19474

Merged

6 tasks

Conversation

epiqueras commented Nov 15, 2019

Description

How has this been tested?

Types of Changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Nov 15, 2019

Uh oh!

epiqueras commented Nov 15, 2019

Uh oh!

epiqueras commented Nov 20, 2019

Uh oh!

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorgefilipecosta Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

epiqueras commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jorgefilipecosta Nov 25, 2019 •

edited

Loading

jorgefilipecosta Nov 25, 2019 •

edited

Loading