-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Change the textcolor passed to ContrastChecker #14716
Conversation
74c5e2f
to
5c08569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgefilipecosta - can you double check the solution proposed?
// This is a feature to improve accessibility. | ||
if ( backgroundColorClass ) { | ||
if ( backgroundColorClass === 'has-white-background-color' ) { | ||
return '#111'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black is encoded as #000
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @gziolo Thanks for your comments. I inspect the CSS files.
When I set the background color to primary, secondary, light gray, dark gray, the CSS file is
so the color is '#fff' (white)
When I set the background color to white, the CSS file is
so the color is '#111' (black)
And you are right, '#000' is also black. But to maintain the consistency, '#111' may be better I think.
@@ -209,7 +223,7 @@ class ParagraphBlock extends Component { | |||
> | |||
<ContrastChecker | |||
{ ...{ | |||
textColor: textColor.color, | |||
textColor: this.getDisplayedTextColor( backgroundColor.class, textColor.color ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon inspection of the file, it seems like it might be necessary to provide defaults inside the implementation of withFallbackStyles
at the top of the file in place of undefined
values.
Hi @Jackie6, @gziolo the root cause of the bug was that withFallbackStyles never recomputed fall back styles again after all fall back styles were known. |
Does it mean we should close this PR and help land the alternative fix? |
I guess that would be the best approach as the solution #14963 is more general while this one more specific to a set of colors used by some themes but may not work in every theme. |
@Jackie6 thanks for working on this PR, let's fly with the proposal from @jorgefilipecosta as it looks more complicated than it seemed on the surface. |
Description
Fix #14687
How has this been tested?
Types of changes
Change the parameters passed to ContrastChecker so that the contrast warning works correctly. That is only when we see light text on light background or dark text on dark background, the warning will be displayed.
Checklist: