-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Update color palette for better accessibility #2490
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.
Looks good. Kudos for such a big improvement in accessibility in one go!
I just want you to confirm if there is any impact on deployment with this new (S)CSS file introduced. I guess once we regenerate it, we should run collectstatic
again?
Do you think it's worth updating README to capture these instructions in case someone wants to extend or modify our SCSS?
@@ -12,12 +15,13 @@ $gray-300: #dee2e6; | |||
$gray-400: #ced4da; | |||
$gray-500: #adb5bd; | |||
$gray-600: #6c757d; | |||
$gray-650: #5a636a; // custom |
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.
Could you perhaps use some calculate function to get this value, for example:
$gray-650: hsl(208, 7%, 46% - 8%); // hsl values for $gray-600
This is an example, as the value you entered for $gray-650
is hsl(206, 8%, 38%)
- very similar to what I put above, but not quite the same.
...
After some thinking I decided to add here that using hsl
or any other color function only makes sense if you modified another color by e.g. increasing or decreasing individual component (like lightness or saturation, or hue). If there's too many components changed then it doesn't really make any sense.
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 switched to hsl, as it's mainly a lightness change (difference of 2 in the other components between gray-600
and gray-700
).
amy/static/css/custom_bootstrap.css
Outdated
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.
Should this filename contain .min
?
// custom_bootstrap.scss | ||
|
||
// compile with Sass: | ||
// $ npm install -g sass |
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'd prefer to put sass
as dev dependency in package.json
. This way everyone following readme will have it installed locally if they need to regenerate this file.
I think we will need to run |
This is achieved by overriding the Sass colors set in Bootstrap 4. I followed the guidance in the Bootstrap 4 docs.
custom_bootstrap.css
andcustom_bootstrap.css.map
are both generated by Sass based oncustom_bootstrap.scss
.Most of the palette is copied from the Bootstrap 5 palette, but some customisation was needed to meet WCAG 2.1 AA contrast throughout, especially for links in striped tables. There is no change to the meaning of any color.
This PR will resolve the majority of contrast-related a11y issues across the whole UI, reducing pa11y failures from ~4400 to ~1300.
Example screenshots which use most of the palette:
Old palette:
New palette: