-
Notifications
You must be signed in to change notification settings - Fork 815
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
Fix button block contrast checker #41294
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
bea3f18
to
4ca3ad4
Compare
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
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.
Thanks for squashing this bug @talldan 👍
I had to rebase this branch for it to build locally for me but once it did everything tested as advertised.
✅ Warning displayed when poorly contrasting colors were chosen
✅ Warning worked regardless of when the combination was user selections, inherited values etc.
✅ Warning was removed when color combination improved or reset to valid values
With the push to improve test coverage in Jetpack, could we add some tests for the hook here as well?
I also left a tiny optimization suggestion as an inline comment. Other than that I think this is looking pretty good.
Screen.Recording.2025-02-28.at.5.18.17.pm.mp4
const fallbackBackgroundColor = getComputedStyle( node ).backgroundColor; | ||
const fallbackTextColor = getComputedStyle( node ).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.
I think we can make a small optimization here by avoiding repeated calls to getComputedStyle( node )
.
4ca3ad4
to
ba59b5a
Compare
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
Thanks for the review. Feedback should all be addressed 👍 |
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.
Nice work @talldan ✨
This is testing well for me. I couldn't break the contrast checker and the tests all pass.
LGTM 🚢

0a96096
to
b59172b
Compare
Proposed changes:
Fixes an issue with the Jetpack Button block's contrast checking. Currently the contrast checker only works when the user has set both the background and text color.
If the user changes only one of the text or background color in a way that causes a contrast issue, the checker doesn't display a warning.
Some history - this functionality was previously handled by a higher order component -
applyFallbackStyles
/withFallbackStyles
. #41139 removed the HOC because it had a few issues:<div>
around the block which breaks a range of stylesThis PR introduces a small hook to replace the
applyFallbackStyles
/withFallbackStyles
to keep the contrast checker working.Other information:
Does this pull request change what data or activity we track or use?
No
Testing instructions: