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.

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

Use Colors: Add text color detection support. #18547

Merged
merged 4 commits into from Dec 2, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 15, 2019

Follows #18237

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. .
@@ -56,7 +56,7 @@ function HeadingEdit( {
</InspectorControls>
{ InspectorControlsColorPanel }
<TextColor>
<BackgroundColorDetector querySelector='[contenteditable="true"]' />
<ColorDetector querySelector='[contenteditable="true"]' />

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 15, 2019

Member

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?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 15, 2019

Author Contributor

Yes, I was just about to ask you that 😄

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 15, 2019

Author Contributor

Done!

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 15, 2019

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

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 20, 2019

@jorgefilipecosta This should be ready to go now 😄

@jorgefilipecosta jorgefilipecosta force-pushed the add/text-color-detection-to-use-colors-hook branch from 53fb172 to a537bc0 Nov 25, 2019
Copy link
Member

jorgefilipecosta left a comment

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.

@@ -29,7 +29,7 @@ function HeadingEdit( {
onReplace,
className,
} ) {
const { TextColor, InspectorControlsColorPanel, BackgroundColorDetector } = __experimentalUseColors(
const { TextColor, InspectorControlsColorPanel, ColorDetector } = __experimentalUseColors(
[ { name: 'textColor', property: 'color' } ],
{
contrastCheckers: { backgroundColor: true },

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 25, 2019

Member

Should we also add textColor: true?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 25, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 25, 2019

Member

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?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 25, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 26, 2019

Member

Yes I guess we can add it 👍

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Author Contributor

Done 😄 9ae645f

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

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 25, 2019

Member

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 comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 25, 2019

Author Contributor

This seems to be working after your last rebase?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 26, 2019

Member

No, I think the warnings still don't appear when expected:
Nov-26-2019 09-04-09

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 comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Author Contributor

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.

Copy link
Member

jorgefilipecosta left a comment

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

@epiqueras epiqueras merged commit 2fd13b6 into master Dec 2, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 2, 2019

Thanks for all the thorough reviews 😄

@youknowriad youknowriad deleted the add/text-color-detection-to-use-colors-hook branch Dec 3, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
@epiqueras epiqueras mentioned this pull request Jan 8, 2020
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.