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.
|
aaronrobertshaw
left a comment
There was a problem hiding this comment.
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.
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 👍 |
aaronrobertshaw
left a comment
There was a problem hiding this comment.
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/withFallbackStylesto keep the contrast checker working.Other information:
Does this pull request change what data or activity we track or use?
No
Testing instructions: