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

Verify if block icon background and foreground colors are readable #7125

Merged
merged 1 commit into from Jun 26, 2018

Conversation

Projects
None yet
2 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jun 4, 2018

If the color combination is not readable, a warning is shown in the developer console.

How has this been tested?

Verify that if the block just sets a background color like https://gist.github.com/jorgefilipecosta/2dd281f9f5f078258f7c8d4ba4cc34cd things work as before.

Verify that if the blocks set a pair of unreadable colors e.g: https://gist.github.com/jorgefilipecosta/3118c641e46fe0bea146877109ccb3af a warning is shown.

Verify that if the blocks set background and foreground colors that are readable no warning is shown https://gist.github.com/jorgefilipecosta/a2eb7c940e008e351f96aadf7ccbd999.

@jorgefilipecosta jorgefilipecosta self-assigned this Jun 4, 2018

@jorgefilipecosta jorgefilipecosta requested review from karmatosed and WordPress/gutenberg-core Jun 4, 2018

@tofumatt

It is likely quite confusing for a user who doesn't have the developer console open to be selecting a colour and have it not take (with a different colour selected) with no message. Should we add some kind of notice more visible to the user for this?

if ( ! icon.foreground ) {
let shouldComputeForeground = true;
if ( icon.foreground ) {
const tinyFgColor = tinycolor( icon.foreground );

This comment has been minimized.

@tofumatt

tofumatt Jun 5, 2018

Member

Can we use foregroundColor or just foreground instead of FgColor here? Abbreviations like this aren't very helpful and it's not like there aren't longer lines 😉

@@ -72,7 +72,20 @@ export function normalizeIconObject( icon ) {
if ( icon.background ) {
const tinyBgColor = tinycolor( icon.background );

This comment has been minimized.

@tofumatt

tofumatt Jun 5, 2018

Member

Can we change this abbreviation too? I know it's unrelated to your changes, but I think the code is easier to read with full words.

) ) {
shouldComputeForeground = false;
} else if ( window ) {
window.console.warn( `The icon background color ${ icon.background } and the foreground color ${ icon.foreground } are not readable together. The foreground color was ignored, and an automatic foreground color computation was used.` );

This comment has been minimized.

@tofumatt

tofumatt Jun 5, 2018

Member

This is about contrast, right? We should explain why the colours aren't readable together so anyone seeing this error understands it. Maybe The icon's foreground color ${ icon.foreground } does not contrast enough against its background color ${ icon.background }. The selected foreground color was ignored, and an automatic foreground color computation was used instead.

Verify if block icon background and foreground colors are readable.
If the color combination is not readable, a warning is shown in the developer console, the foreground color is ignored and an automatic computation of the color is used instead.
@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 22, 2018

It is likely quite confusing for a user who doesn't have the developer console open to be selecting a colour and have it not take (with a different colour selected)

Hi @tofumatt thank for the review, I updated the code, now when colors are not readable, we show a warning in the console and but we don't choose another color. This is more in line with decisions made in other places like the color palette warnings.

@tofumatt

Awesome! Sorry for the delayed review; this is much nicer 👍

@jorgefilipecosta jorgefilipecosta merged commit 8fa8b64 into master Jun 26, 2018

2 checks passed

codecov/project 46.81% (+0.01%) compared to b16a8b2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/implemented-verification-of-background-and-foreground-readibility branch Jun 26, 2018

@jorgefilipecosta jorgefilipecosta added this to the 3.2 milestone Jul 5, 2018

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