Closed jcarstairs-scottlogic closed 10 months ago
For next time/larger project, I would recommend splitting the refactoring changes and functional changes into separate PRs. Why we might want to have the overhead of two PRs is that it would let technical people quickly approve and merge the technical improvements in isolation, which could lead to a quicker elapsed time in making the changes available to other developers.
In this case, a hypothetical discussion about "how grey do we want our default grey to be? Do we want to keep a separate lighter grey?", might delay approval if it required further input from e.g. a product owner or a UX specialist.
"In contrast, dart-sass should be plug-and-play..."
When you say "should be" it can make paranoid colleagues concerned that this hasn't actually been tested!
I'm assuming that you've run it on windows, and that it's successfully run on linux as part of the deployment process, so we actually know it works on two out of three. You could also try adding a cheeky request for anyone reviewing this PR and who coincidentally has a mac to give it a try!
After all your hard work investigating the color contrasts, as we don't yet have a timeframe for a future fix, I'd recommend putting a warning, just summarising your contents above, in e.g. the colors.scss file,
I agree with the change to make all grey text the darker shade, but even though it's no longer used in any fonts, I would have probably have kept the light grey color definition as a potential background color. (Although I'm also guessing that font-grey is not going to have enough color-contrast over a light grey background either!).
FWIW I've been able to run sass first time with a local installation of this branch.
@jcarstairs-scottlogic is there a deployed branch where I can visually check these changes? thanks.
@jcarstairs-scottlogic is there a deployed branch where I can visually check these changes? thanks.
Updates node-sass to dart-sass
node-sass is deprecated. dart-sass is now the recommended implementation of Sass.
Both myself and @ithake have had difficulty running node-sass on Windows. Ian did manage it in the end, but it took a couple hours of wrangling. In contrast, dart-sass should be plug-and-play on Windows, Linux and Mac.
Encapsulate colours
Uses the SCSS
@use
rule to encapsulate all the blog's core colours in one file,_colours.scss
. (Syntax highlighting is already encapsulated in_syntax-highlight.scss
, I've left that alone.) This makes any future changes to colours easier to implement.Fix grey text colour contrast ratio
There are two grey text colours which previously had inadequate colour contrast.
One instance is the dates underneath talks in a list of talks. This had a 3.94:1 colour contrast ratio (4.5:1 is required for WCAG AA compliance).
A second instance is the category above a blog post in a list of blog posts. This had a colour contrast ratio of 2.02:1.
Both greys have now been set to
#767676
using a new SCSS variable,$grey
, in_colours.scss
. This grey has adequate colour contrast against a white background, which is where the greys are used. This is what they look like after the fix:If someone ever wanted to use grey in future in a different context, they might have to choose a different grey. It has inadequate colour contrast against a light teal background, against a light orange background, against a light pink background and against a light yellow background. These are all used as background colours in parts of the blog. However, as the grey text currently only appears on a white background, I thought this wasn't an important consideration.