-
Notifications
You must be signed in to change notification settings - Fork 17
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
Improve contrast between some texts and their backgrounds #644
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.
Thanks for testing all this!
frontend/src/ui/Blocks/Series.tsx
Outdated
@@ -268,7 +268,7 @@ const GridTile: React.FC<GridTypeProps> = ({ event, basePath, active }) => { | |||
}}>{event.title}</div> | |||
</h3> | |||
<div css={{ | |||
color: "var(--grey40)", | |||
color: "var(--grey37)", |
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 would just use --grey20
then. Also, this UI element will change when we apply the redesign, so this is somewhat temporary anyway.
048deea
to
bc94017
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.
Great work on the contrast calculation! I left two inline comments and have to additional ones:
-
Git stuff got mixed up a bit. In the first commit you still change
--happy-lightness
and in the second one you revert it. But the first commit is called "Revert ...". So I think you meant to only change thegrey20
in the first commit (and have an appropriate commit message) and only do the contrast stuff in the second. -
Not really all that necessary but as a learning opportunity for you and because this is one of the few things that's easy to test: do you want to add some unit tests for the
bw_contrast
function? Just test it with a few color combinations (in particular also the one where it previously failed and which made us rewrite the function). You can put the tests simply at the bottom of this file, usually like this:#[cfg(test)] mod tests { ... }
The rest you can probably learn fairly easily on the web, as you did with the other stuff required to write
bw_contrast
. If not, just ask, of course.
On video pages, the selected video in the current series has a darker background than the others. This makes the author and upload and starting time line a little harder to read and the contrast ratio would not pass accessibility checks. This will be changed with the upcoming redesign, but as an intermediate solution, this commit changes the used color for these lines to a darker shade of grey.
This changes the contrast function to conform to WCAG2.1 guidelines. The calculations of relative luminance and contrast ratio now follow the procedure outlined in "https://www.w3.org/WAI/WCAG21/Techniques/general/G18.html", which appears to be the broadly used standard for most accessibility contrast checking tools. Includes tests for base theme colors.
bc94017
to
d7bb396
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.
Excellent work!
Thank you, and sorry for not changing the title myself! I did realize at some point that "accessibility review" wasn't really fitting, but unfortunately forgot to adjust it. |
These are two mini-fixes to address some of the contrast errors found by the WAVE browser plugin.
I also tested most if not all tobira pages using tab navigation and voice over.
The
AddButtons
are not focusable using tab, but that appears to be fixed with #636.The only other issues I noted so far is on video pages: Starting videos works fine using the keyboard, but once the control elements disappear, they seem to also lose focus, which makes it impossible to refocus them with the tab button. But I suppose this is rather a problem with the embedded player than tobira.
Also, the
play button
appears to be missing a description and is simply read asbutton
, ortaste
in german.