color contrast check prototype #349
Conversation
|
Variables changedNo variables changed |
🟢 No design token changes found |
|
I added all color pairs now. However the lib does not work with transparent colors. This is something we need to address. The "problem" here is that we may transparent colors on multiple bgs. E.g. a This should work but adds more effort. @rezrah do you have any other idea? |
|
Okay, I added the possibility to define multiple bgs for transparent colors. The way it works now:
|
|
Hey @rezrah I think this is ready now. Also this script outputs |
| * testContrast | ||
| * @description test the contrast of two colors against each other | ||
| * @param minimumContrast | ||
| * @param colorA |
There was a problem hiding this comment.
can we refer to the params as foreground and background for clarity?
There was a problem hiding this comment.
We could, but this would not be truthful for testing e.g. default text color vs. link color. I had fg vs. bg first.
Happy to adjust it again, what do you think?
| */ | ||
| const renderConsoleTable = (theme: string, results: contrastTestResult[]): void => { | ||
| // config table | ||
| const contrastTable = new Table({ |
There was a problem hiding this comment.
Any chance we can do this without the dependency? Appreciate it's the quicker option, but it leaves us open to security and maintenance burden over time that becomes difficult to justify for otherwise straightfoward tasks. Happy to help rewrite this in a part 2 PR if that helps too!
There was a problem hiding this comment.
Yeah, lets book this for a 2nd PR please.
Reasons for the dep:
- Table looks better
- center aligned columns with
console.tableare hard to read
There was a problem hiding this comment.
Table looks better
FYI.. worth bearing in mind that comments in the Pull Request will strip out all extra styling
There was a problem hiding this comment.
But not when using it locally.
There was a problem hiding this comment.
This is great @lukasoppermann 🙌
- To answer your question in description, I can definitely work with this in CI workflows
- Left some comments on things we should fix before this merges. Typing errors, deps etc.
- Happy to help fix those with you or in future PR if easier too. LMK.
Should a build fail if contrasts fail?
IMO no, because this is a side-effect and additional abstraction on top of build jobs. It would be inaccurate to suggest the build failed when it was actually a governance layer that did. We can work around it by making the contrast check fail instead for transparency.
Instead, let's focus on failing package releases:
- Canary should always pass and ship, to avoid blocking testing
- GA on
@latestchannel should fail
Thoughts?
lukasoppermann
requested review from
rezrah
and removed request for
langermank
This is just the start. Once merged it will fix https://github.com/github/primer/issues/1277 Many questions are popping up:
token.json? In a separate spec file?)ignoreflag that will report the fail but ignore it)Todo:
'around strings)