Contrast Checker: check link color #38100
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
[Type] Enhancement
|
Size Change: +270 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
also would resolve #24595 |
ramonjd
force-pushed
the
try/check-link-color-in-contrast-checker
branch
2 times, most recently
from
fcd2ff8
to
89651ee
Compare
ramonjd
commented
Jan 21, 2022
ramonjd
commented
Jan 24, 2022
ramonjd
commented
Jan 24, 2022
packages/block-editor/src/components/contrast-checker/contrast-checker-message.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for your work on this @ramonjd 🙇
The code changes make sense to me and it all tests as advertised. The example block content was a very helpful starting point and a nice touch, so kudos for that 👍
✅ Unit tests pass
✅ Continues to trigger contrast warnings for background and text colors appropriately
✅ Contrast warnings for links work as advertised
✅ Transparency warnings work as advertised also
As discussed by Glen and yourself previously, I think this is in a good place for a little bit of a refactor, then it should be right to land.
I have one small question though regarding the wording of the contrast warning now it also encompasses link colors but whether it warrants tweaking I'll leave up to you.
packages/block-editor/src/components/contrast-checker/contrast-checker-message.js
Outdated
Show resolved
Hide resolved
|
Thanks a lot for the review @aaronrobertshaw and @glendaviesnz !!
👍 I'll try to untangle things. Maybe it's possible to iterate over a set of rules or something a little more readable. |
…sing it to the ConstrastChecker Added base logic and tests. Very messy. To be optimzized. Don't hate me.
… colors. This is so it can handle checking multiple text/foreground colors on a single background, e.g., text, link and so on. Updating arguments and tests.
ramonjd
force-pushed
the
try/check-link-color-in-contrast-checker
branch
from
9a2c5a1
to
00e5e31
Compare
Done. Not sure if it's more readable, but it reduces the logic nightmare and opens the checker up to more than two text/foreground colors now.
The refactor includes sending custom messages so we can tweak the copy depending on the which text triggers the warning. |
|
Actually I need to wind things back a bit. Maybe keep the original props for now as I haven't looked at the mobile files yet. Also the message component can probably be reintroduced into the main component given its size. |
…be done iteratively as it touches too many parts, e.g. mobile.
|
✅ Maybe keep the original props for now as I haven't looked at the mobile files yet. |
aaronrobertshaw
approved these changes
Feb 7, 2022
There was a problem hiding this comment.
Thanks for continuing to iterate on this @ramonjd 👍
The approach in keeping the original props makes sense and isolating any refactor to a follow-up sounds good to me.
The inclusion of this will have a small impact on the PR switching the color panel to use the ToolsPanel as well. I'm only noting that here in case for some reason this PR doesn't land first.
I've left a few minor nitpick comments but I'll approve this in case you'd prefer to dismiss those suggestions.
ramonjd
commented
Feb 7, 2022
Co-authored-by: Aaron Robertshaw <[email protected]>
Update comment so that it reflects what's going on in the code.
priethor
added
[Focus] Accessibility (a11y)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #24595
Description
Hi! Let's detect the color of an A tag and, if it exists, pass it to the
<ConstrastChecker />There are WACG guidelines specifically for links:
See: https://webaim.org/articles/contrast/#sc141
To keep things straight forward we're applying the same AA standard to links that we use for text. We can make our tests more stringent should the debate take us there.
How has this been tested?
Insert a Paragraph block, and some nested blocks that all have color controls, for example, a Paragraph block within a Group block.
Play with text, background and alpha colour controls in the Block Inspector sidebar.
👀 Here is some test content containing various scenarios:
Test blocks
The contrast warning should show when:
The contrast warning should take priority over the transparency warning. That is, if the text color for example has poor contrast and the link color has transparency, the contrast warning will show.
The transparency warning should show when:
Check the text as well. We can pass a description so contrast warnings will specify 'link' if a link triggers a warning.
The tests should cover the scenarios as well:
npm run test-unit packages/block-editor/src/components/contrast-checker/test/index.jsKnown issues
This is an existing issue. I'm noting here so it doesn't distract from testing :)
The contrast checker will test and warn about unreadable global styles colors even though the user hasn't interacted with the panel. For example, if you have the following color definitions in your theme.json,
You'll see the warning as soon as you open the panel:
Follow up work.
The Social Icons block tests icon colors and backgrounds. We could test passing an array of colors as a prop, as we did in 00e5e31 to get custom messages.
I reverted 00e5e31 because I thought the changes were too sweeping and needed more testing across the app. We would have to update mobile as well.
Context
Related issue #37549
Discussed in #37731 (comment)
Global styles issue found in #35774 (comment)
Types of changes
Enhancing existing feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).