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

Added contrast verification to paragraph colors #3483

Merged
merged 5 commits into from Nov 24, 2017

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Nov 14, 2017

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):

nov-14-2017 17-07-04
nov-14-2017 17-07-19

@jorgefilipecosta jorgefilipecosta self-assigned this Nov 14, 2017

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

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 15, 2017

Member

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.

Member

gziolo commented Nov 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 15, 2017

Member

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 :)

Member

jorgefilipecosta commented Nov 15, 2017

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 :)

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

This comment has been minimized.

@gziolo

gziolo Nov 16, 2017

Member

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.

@gziolo

gziolo Nov 16, 2017

Member

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 16, 2017

Member

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.

@jorgefilipecosta

jorgefilipecosta Nov 16, 2017

Member

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.

This comment has been minimized.

@gziolo

gziolo Nov 16, 2017

Member

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

every( newFallbackStyles )

It might better express intent.

@gziolo

gziolo Nov 16, 2017

Member

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

every( newFallbackStyles )

It might better express intent.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 16, 2017

Member

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.

@jorgefilipecosta

jorgefilipecosta Nov 16, 2017

Member

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

@gziolo

gziolo Nov 17, 2017

Member

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.

@gziolo

gziolo Nov 17, 2017

Member

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 24, 2017

Member

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

@jorgefilipecosta

jorgefilipecosta Nov 24, 2017

Member

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

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 16, 2017

Member

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

Member

gziolo commented Nov 16, 2017

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

@gziolo gziolo requested a review from karmatosed Nov 16, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 16, 2017

Codecov Report

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

Impacted file tree graph

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

codecov bot commented Nov 16, 2017

Codecov Report

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

Impacted file tree graph

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

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 16, 2017

Member

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

Member

jorgefilipecosta commented Nov 16, 2017

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

gziolo approved these changes Nov 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Nov 23, 2017

Member

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

Member

karmatosed commented Nov 23, 2017

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

@jorgefilipecosta jorgefilipecosta merged commit a432882 into master Nov 24, 2017

3 checks passed

codecov/project 37.09% (-0.06%) compared to bccede6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/contrast-verification-paragraph branch Nov 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment