Added contrast verification to paragraph colors #3483

jorgefilipecosta · 2017-11-14T17:23:49Z

Description

This PR aims to address the remaining part of #2381, by adding a contrast checking following WCAG 2.0- AA (https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.html) to the paragraph.

How Has This Been Tested?

Test some colors in the paragraph and verify that when contrast is high no message, in low contrast color pairs a message appear.
Verify that when the background is darker than text, we suggest making the background darker and/or the text lighter and when text is darker we suggest making the text darker and/or background lighter.
Verify that for some color pairs e.g. background #C46161 and black text or our blue with no background the message does not appear, if the font size is equal or larger than 18 and the message appears if the font size is smaller.

Screenshots (jpeg or gifs if applicable):

gziolo · 2017-11-15T07:09:12Z

blocks/library/paragraph/index.js

+		this.grabTextColor();
+	}
+
+	grabTextColor() {


@jorgefilipecosta have you considered factoring out color related logic into a higher-order component? It looks very familiar to the implementation of the button. In both cases, we have text color and background color that are compared. From what I see this wrapper component could handle:

node

editableRef

state

bindRef

grabTextColor

componentDidMount & componentDidUpdate

gziolo · 2017-11-15T07:13:23Z

blocks/library/paragraph/index.js

+					</PanelBody>
+					<ContrastChecker
+						textColor={ textColor || fallbackTextColor }
+						backgroundColor={ backgroundColor || '#fff' }


Is there a plan to add fallback logic to backgroundColor, too? Or maybe it is always white anyway :)

That's a nice question, I used a logic for background color but, the getComputeStyle always returned black, while the background is white If background color was explicitly set the getComputedSyle returned correctly. So I assumed the background when not explicitly set is white.

Better try to change it using CSS. It is possible that an external plugin would apply such change.

gziolo · 2017-11-15T07:17:09Z

This is a really cool feature 💯 The code looks good from what I can see, I left a suggestion to extract a higher order component to hide the logic related to the color pickers. Anyway if you feel that the current solution works better in this case, let me know and I will do more in-depth code check.

jorgefilipecosta · 2017-11-15T22:37:06Z

Hi @gziolo, I followed your advice and I created a High Order Component to abstract part of the common logic withFallbackStyles. Some parts change from block to block and I was not able to abstract.
If we decide to follow this approach I will add a readme and tests to the HoC. Feel free to have a new look :)

gziolo · 2017-11-16T08:23:02Z

blocks/library/button/index.js

+const ButtonBlockFallbackStyles = withFallbackStyles( ( node, ownProps ) => {
+	const { textColor, color } = ownProps.attributes;
+	return {
+		fallbackBackgroundColor: color ? undefined : getComputedStyle( node.querySelector( `.${ ownProps.className }` ) ).backgroundColor,


Alternatively, you can use span[key="button"] as a query selector.

Hi @gziolo, I tried this approach, but unfortunately react element keys are not passed to the dom (possible to confirm using browser dev tools), so this query selector was not returning and I end up not applying this change.

Fair enough 👍

gziolo · 2017-11-16T08:27:11Z

blocks/library/paragraph/index.js

+	const { textColor, backgroundColor } = ownProps.attributes;
+	const computedStyles = ( ! textColor || ! backgroundColor ) ? getComputedStyle( node.querySelector( '[contenteditable="true"]' ) ) : null;
+	return {
+		fallbackBackgroundColor: backgroundColor ? undefined : computedStyles.backgroundColor,


Can you explain why we set undefined when color is passed as a prop? From what I can see we can safely set it, but it will be ignored. It would make reading code easier if we would always compute it even when it's not used. Unless I missed something.

If there are custom colors set the fallbackColors will be equal to the custom colors set. As we have a logic that stops grabbing colors when we have all fallbackColors needed, if later we remove the custom colors, the fallbackColors are not updated and would still be equal to the custom colors set the first time instead of the colors that appear when no custom color is selected.

gziolo · 2017-11-16T08:28:27Z

blocks/library/paragraph/style.scss

@@ -18,3 +18,7 @@ p.has-drop-cap:not( :focus )  {
 p.has-background {
 	padding: 20px 30px;
 }
+
+.wp-block-paragraph {


@jasmussen or @karmatosed, is it okey to set it explicitly here?

gziolo · 2017-11-16T08:36:21Z

components/higher-order/with-fallback-styles/index.js

+
+		grabFallbackStyles() {
+			const { grabStylesCompleted, fallbackStyles } = this.state;
+			if ( this.nodeRef && ! grabStylesCompleted ) {


Can you explain why we need to use grabStylesCompleted in here? Can we check if fallbackStyles is still undefined instead? It looks like you can always compute grabStylesCompleted using the existing variables.

Hi @gziolo, for example in button we may have set a custom text color, so we can not compute the fallbackTextColor but we can compute the fallbackBackgroudColor. So our mapNodeToProps returns undefined for the properties it cannot compute the fallback color (fallbackTextColor). When there is no undefined value ( ! findKey( newFallbackStyles, ( obj ) => obj === undefined ) ) = true we know we don't need to grab colors anymore. The value of this is saved in grabStylesCompleted so we are not running this line with findKey all the time.

Another advantage is when inserting a component no custom colors are set so all fallback colors are immediately saved and logic to grab color is just executed one time.

Would it work if you would use every from lodash instead of findKey:

every( newFallbackStyles )

It might better express intent.

I agree using "every" would make things simpler to understand, but I think we can not use it here. Every works on a collection here we are working a single object and we just want to make sure there is no key/property of that object that is undefined.

This is a perfectly fine usage

every( { a: true, b: 1, c: undefined } );

It works with with both arrays and objects:

collection (Array|Object): The collection to iterate over.

Some utility methods that operate on collections, work with strings, too. See includes.

Thank you for your suggestion @gziolo you were right and your suggestion was applied before merging.

gziolo · 2017-11-16T09:21:37Z

It works perfectly fine, great job. Thanks for doing all refinements.

Now, seeing all that logic extracted, I started to think if we can take a slightly different approach and wrap HOC directly on < ContrastChecker /> component.

What if you would be able to use it as follows:

<ContrastCheckerWithFallbackStyles
	node={ this.nodeRef } 
	textColor={ textColor }
	backgroundColor={ backgroundColor }
	isLargeText={ true }
	/>

This way the contrast checker would only have a reference to the node to compute fallbacks when necessary.

Anyway, I'm also fine with what we aleady have here. 👍

codecov · 2017-11-16T13:18:12Z

Codecov Report

Merging #3483 into master will decrease coverage by 0.05%.
The diff coverage is 8.92%.

@@            Coverage Diff             @@
##           master    #3483      +/-   ##
==========================================
- Coverage   37.15%   37.09%   -0.06%     
==========================================
  Files         275      276       +1     
  Lines        6629     6653      +24     
  Branches     1202     1210       +8     
==========================================
+ Hits         2463     2468       +5     
- Misses       3517     3528      +11     
- Partials      649      657       +8

Impacted Files	Coverage Δ
blocks/contrast-checker/index.js	`0% <0%> (ø)`	⬆️
...ponents/higher-order/with-fallback-styles/index.js	`11.11% <11.11%> (ø)`
blocks/library/button/index.js	`14.7% <14.28%> (+5.4%)`	⬆️
blocks/library/paragraph/index.js	`23.07% <7.14%> (-6.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bccede6...22d59ba. Read the comment docs.

jorgefilipecosta · 2017-11-16T13:22:56Z

Now, seeing all that logic extracted, I started to think if we can take a slightly different approach and wrap HOC directly on < ContrastChecker /> component.

Hi @gziolo, I did a new commit that uses this approach instead :) It has the advantage of not color grabbing logic being executed at all if contrast verification is not needed. No changes were needed in our HoC, and the only change applied in the contrast verifier was adding the fallbackColors properties directly to it (optional props).

gziolo

I’ll test again tomorrow, it looks great at the moment. I really like how independent contrast checker component became 👏

gziolo

Code wise looks good to me. It also works as advertised.

I would also suggest asking @afercia or @nic-bertino to verify if it works as expected before you merge it.

karmatosed · 2017-11-23T16:22:18Z

Let's get the conflicts resolved on this and then merge, it's better than what we have right now so worth getting in.

…les higher order component.

…llbackStyles wrapper.

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Nov 14, 2017

jorgefilipecosta self-assigned this Nov 14, 2017

jorgefilipecosta changed the title ~~Added contrast verification paragraph~~ Added contrast verification to paragraph colors Nov 14, 2017

gziolo reviewed Nov 15, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/contrast-verification-paragraph branch 6 times, most recently from c7bdf26 to 4d03540 Compare November 15, 2017 22:18

gziolo reviewed Nov 16, 2017

View reviewed changes

gziolo requested a review from karmatosed November 16, 2017 09:24

jorgefilipecosta force-pushed the add/contrast-verification-paragraph branch 2 times, most recently from 4564831 to 54ac133 Compare November 16, 2017 13:18

gziolo reviewed Nov 16, 2017

View reviewed changes

gziolo approved these changes Nov 17, 2017

View reviewed changes

jorgefilipecosta added Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement. and removed [Status] In Progress Tracking issues with work in progress labels Nov 20, 2017

karmatosed approved these changes Nov 23, 2017

View reviewed changes

Paragraph block: refactored paragraph edit block to be a class.

d602f54

jorgefilipecosta force-pushed the add/contrast-verification-paragraph branch 3 times, most recently from e9bd575 to 5e7d630 Compare November 24, 2017 16:17

jorgefilipecosta added 4 commits November 24, 2017 16:34

Created a withFallbackStyles higher order component.

0feb98d

Paragraph block: Added contrast verification.

c654e80

Button Block: Refactored button fallback color to use withFallbackSty…

6379935

…les higher order component.

Refactored contrast verification logic to use a ContrastCheckerWithFa…

22d59ba

…llbackStyles wrapper.

jorgefilipecosta force-pushed the add/contrast-verification-paragraph branch from 5e7d630 to 22d59ba Compare November 24, 2017 16:35

jorgefilipecosta merged commit a432882 into master Nov 24, 2017

jorgefilipecosta deleted the add/contrast-verification-paragraph branch November 24, 2017 16:43

ellatrix mentioned this pull request Dec 13, 2017

Try multi-select inspector for same blocks #3535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added contrast verification to paragraph colors #3483

Added contrast verification to paragraph colors #3483

jorgefilipecosta commented Nov 14, 2017

gziolo Nov 15, 2017

gziolo Nov 15, 2017

jorgefilipecosta Nov 15, 2017

gziolo Nov 15, 2017 •

edited

Loading

gziolo commented Nov 15, 2017

jorgefilipecosta commented Nov 15, 2017

gziolo Nov 16, 2017

jorgefilipecosta Nov 16, 2017

gziolo Nov 16, 2017

gziolo Nov 16, 2017

jorgefilipecosta Nov 16, 2017

gziolo Nov 16, 2017

gziolo Nov 16, 2017

jorgefilipecosta Nov 16, 2017

gziolo Nov 16, 2017 •

edited

Loading

jorgefilipecosta Nov 16, 2017

gziolo Nov 17, 2017 •

edited

Loading

jorgefilipecosta Nov 24, 2017

gziolo commented Nov 16, 2017

codecov bot commented Nov 16, 2017 •

edited

Loading

jorgefilipecosta commented Nov 16, 2017 •

edited

Loading

gziolo left a comment

gziolo left a comment

karmatosed commented Nov 23, 2017

Added contrast verification to paragraph colors #3483

Added contrast verification to paragraph colors #3483

Conversation

jorgefilipecosta commented Nov 14, 2017

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo commented Nov 15, 2017

jorgefilipecosta commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 16, 2017

codecov bot commented Nov 16, 2017 • edited Loading

Codecov Report

jorgefilipecosta commented Nov 16, 2017 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

karmatosed commented Nov 23, 2017

gziolo Nov 15, 2017 •

edited

Loading

gziolo Nov 16, 2017 •

edited

Loading

gziolo Nov 17, 2017 •

edited

Loading

codecov bot commented Nov 16, 2017 •

edited

Loading

jorgefilipecosta commented Nov 16, 2017 •

edited

Loading