-
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
ContrastChecker: Add font size logic inside the component; Add additional test cases; #8059
ContrastChecker: Add font size logic inside the component; Add additional test cases; #8059
Conversation
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.
Works well and the code lgtm. Thanks for adding tests.
I'm wondering if we should just eliminate the isLargeText
prop altogether. It looks like it's only used in one place now:
gutenberg/core-blocks/button/edit.js
Line 123 in a0724b7
isLargeText={ true } |
We could replace this remaining usage with fontSize={ 18 }
since we know that buttons have 18pt text per this line:
gutenberg/core-blocks/button/style.scss
Line 13 in a0724b7
font-size: $big-font-size; |
Anyway, not a blocker—totally up to you 🙂
Contrast checker included logic to differentiate between large and small text. The logic is dependent on fontSize so it makes sense to have for normal cases logic that uses the fontSize inside ContrastChecker checker. But because the logic is not just dependent on fontSize (e.g: if text is bold for the same fontSize the contrast may be valid) we allow users of the component to still rely on isLargetext prop and decide by them selfs if the text they contain is considered large or not.
4edac18
to
f7d752e
Compare
Hi @noisysocks thank you for the review. I think the property isLargeText should exist because as referred font size is not the only decision point if the text is bold for example it may be considered large with a different font size. |
@jorgefilipecosta thanks for your work on this! I have a question though: is the font size check based on pixels or points? Please notice that the WCAG requirement refers to points, not pixels. Reference: https://www.w3.org/TR/WCAG21/#contrast-minimum
"Large scale text" is defined in points, not pixels:
So, calculating the actual point size may depend on various conditions. However, it's an (more or less) accepted practice to use 24 pixels and 18.5 pixels as values. This is explained in the WCAG "Understanding Success Criterion 1.4.3: Contrast (Minimum)": https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html
Please let me know if there's the need to open a new issue, thanks! |
Contrast checker included logic to differentiate between large and small text. The logic is dependent on fontSize so it makes sense to have for normal cases logic that uses the fontSize inside ContrastChecker checker. The logic is not just dependent on fontSize (e.g: if text is bold for the same fontSize the contrast may be valid) we allow users of the component to still rely on isLargetext prop and decide by them selfs if the text they contain is considered large or not.
How has this been tested?
I created a paragraph I set the text color to "#C44B4B" and the background color to "#000000".
I checked that for font sizes medium an lower there is a contrast waring and for large font sizes, there is no contrast warning.