WIP Repaint the bike shed #9916
Conversation
chris48s
added
core
There was a problem hiding this comment.
pinned issue/discussion ... blog post
+1! I was going to offer to write one if you hadn't. 😄
Speaking from past experience: there's a good chance some users are going to be very upset about this change being hoisted on them. The basic morality of accessibility won't change their minds. They'll want the way it was "working" before, and will think it's presumptuous and rude of the badges/shields team to hoist changes on them.
IME a good way to avoid this kind of rage is to:
- Give people a way to opt into the legacy behavior
- Make that way force them to understand why the legacy behavior is bad
- When a tiny % of devs opt into the legacy behavior, use that as justification for removing it in a few months
I.e. here it could be a dull gray/red checkbox on the shields generation page with a link to the blog post.
How does that strike you?
There was a problem hiding this comment.
I'll wait for an opinion from other maintainers, but I think.. not this.
We probably could add a ?palette=legacy or something, but I think I am in favour of just ripping the band-aid off once, rather than having to do it twice. In general, with a service it is hard to gradually deprecate a param in the same way you can with a library. For shields.io users ( https://shields.io/ itself), we don't really have a good way to communicate changes to users. We don't know who our users are (because we're not tracking them) so I think I'd be against introducing any param we plan to remove.
I guess one argument for the legacy palette would be that lots of people use shields badges alongside badges from elsewhere e.g:
and you might want to have consistency.
I think I'm OK with not providing a legacy palette and shields.io "leading the way" on this.
As I say, I'll wait for others to weigh in.
There was a problem hiding this comment.
👋 just checking in @chris48s, is there anything I can do to help move this forward?
There was a problem hiding this comment.
@chris48s how much burden would you say we'd have (codebase, runtime, issue maintenance, etc.) from any attempts to support both?
There was a problem hiding this comment.
I have not thought about this PR for a while, but I will come back to this and think it through. Just to help me answer this question, what's the scenario we're imagining?
- We keep 2 palettes forever, or
- We have both for a while, then at some point we drop legacy?
There was a problem hiding this comment.
Having thought about this...
I don't think the code to enable this would be super-complicated.
I've not tried implementing it, but I think the code to add another param, pass it down and use it in the right place would not be a huge diff. It introduces another global optional param and another library param to document. The core rendering path is code we change relatively infrequently though. As such, I think that represents a relatively fixed overhead from a code perspective. We do it once and it doesn't have much of a maintenance burden.
One thing it does complicate a bit is: anywhere we use a custom colour, we need to be aware that it might appear in the context of 2 palettes.
I don't think it makes a big difference at runtime. I can't see it meaningfully impacting performance, for example.
I think the biggest overhead with doing this would be the documentation, communication and support around it. That tends to be the most labour intensive part on an ongoing basis for something like this. We can document stuff as much as we like, but we'll still have to field support requests about it forever. Unfortunately this is harder to quantify but any global query param tends to raise a fairly steady stream of queries over time.
If we do envisage going down the route of adding a ?palette=legacy param to opt into the legacy behaviour, we could go with @JoshuaKGoldberg 's suggestion and:
- introduce
?palette=[accessible|legacy]but leavelegacyas the default to start with - publicise that
?palette=accessibleexists and ask for feedback - almost nobody will opt in to start with, but it gives us the opportunity to iron out any unforeseen problems (e.g: further tweaks to the palette) with a small group of enthusiasts before it becomes the default - having ironed out any issues, we announce/document that this will become the default on date X
- one date X flip the default to
?palette=accessibleand you can opt out with?palette=legacy
There was a problem hiding this comment.
I have not thought about this PR for a while, but I will come back to this and think it through. Just to help me answer this question, what's the scenario we're imagining?
* We keep 2 palettes forever, or * We have both for a while, then at some point we drop legacy?
Sorry i forgot to respond to this, but I was originally thinking forever
There was a problem hiding this comment.
Just confirming: what does this look like for very pale colors such as aqua, beige, and lime?
There was a problem hiding this comment.
So those 3 would look like:
As I say in the top post
- I'm not attempting to achieve sufficient contrast in 100% of cases. There will still be cases where a user can supply a named CSS colour or hex value (either on a custom badge, or as an override) outside the standard palette that decrease accessibility.
- I'm not making any changes to the brightness calculations we use to automatically switch the text or logo colour when using custom colours.
I think those things definitely are worth looking at, but I think they should be follow-up PRs IMO.
|
Total aside: I'd started working on this (https://github.com/JoshuaKGoldberg/shields/tree/render-shadow) and had a hard time getting the local dev server to show my changes. Very happy you're taking this on. Thanks 😄 |
|
Haven’t tested this, but I'm on board with the spirit of the changes. At first glance, I'll note that "brightgreen" doesn't feel very bright anymore, and yellow feels a little brown-ish. |
|
Yeah there's a bit of discussion about the brightgreen here #5497 (comment) Obviously there's a tension between darkening sufficiently to improve contrast while retaining bright/light-ness |
| @@ -4,15 +4,15 @@ const { fromString } = require('css-color-converter') | |||
|
|
|||
| // When updating these, be sure also to update the list in `badge-maker/README.md`. | |||
There was a problem hiding this comment.
In the PR description you did mention:
This PR makes changes to the badge-maker NPM package. I haven't bumped the package version or updated the changelog in this PR.
so just wanted to add the readme file to that list of "to be updated" given this comment
This was the first thing that jumped out at me as well. The other feeling I had reading back over this (and the issue) is that I can't visually differentiate between the changed green and brightgreen any more. That may just be my screen and/or my eyes, and even though large chunks of the discussions went over my head, the reasoning and calculations behind those seem valid and reasonable so I'm definitely not objecting. That's just my human reaction with a "shields user" hat on that I can't readily tell the difference anymore. I was trying to think of cases where we have badges with a default color scale that includes both, and I think that's largely found in places like rating/score and code coverage badges with a clear textual value that'll help (e.g. my green coverage badge with 99% coverage may have a background color I can't readily differentiate, but the 100 vs 99 percentage text would definitely avoid me misreading the badge) |
|
While looking at another issue I noticed Line 92 in caea759 Just chucking a link to this here to remind myself when I get a chance to come back to this. |
I think there might be a bit of an iron triangle here. There are several properties we want to optimise for:
and it is proving tricky to find a solution that achieves all 3 of those at the same time. I guess one question here is: We've concentrated on calculating the contrast between the background and the text, but we haven't really looked at the contrast/difference between the different colours in the scale (at least not quantitatively). Is there any kind of guidance or formula we can use that might inform how different colours in a scale need to be in order to be sufficiently different from each other, accepting that they might not actually appear together?
Yes. This is good feedback though. The entire point of this is to try and make stuff readable on a wider range of screens/eyes.
There are a number of palettes where we use "green" and "brightgreen" in the same colour scheme. If you have a look over https://github.com/badges/shields/blob/master/services/color-formatters.js but yeah coverage and ratings would be examples. |
Yes, and it'd be the same contrast algorithm as other parts. If there is enough color contrast between the two colors using whatever algorithm you choose, they're separate; if not, they would be considered as not being separate. The same APCA resources apcacontrast.com mentioned #5497 (comment) can work in theory, except it looks like the website is down right now. 🥲 |
|
Here's another thought on the green/brightgreen thing.
so.. what if this is telling us: this should be one colour? What if we got rid of the distinction between green and brightgreen and the scale went
and brightgreen becomes a legacy alias for green I feel like that requires a bit more of a lift because we change the number of possible buckets we can put a score into and we have to review any scale that currently uses both green and brightgreen. It might also make it more difficult to maintain 2 palettes. I've not completely thought that through, but what do we think of that as an idea? Just reduce the number of colours in the palette. |
I think you're onto something here, though the devil will of course be in the details. I suspect there will also likely need to be a consideration of the different personas (persona A being a long time shield user who is accustomed to the prior palette and feel like their 100% coverage is visually "wrong", and persona B being a new user who's 👍 with the palette and appreciates the contrast and accessibility aspects) |
|
It has been 6 months since I last worked on this branch, so it is not really fresh in my mind, but I will try and find some time to get my head back into this and see what's involved in reducing the colour palette so there is only one green. |
|
OK. It has been a really long time since I looked at this, but I spent a bit of time combing though everything to work it out. My assessment here is that going down to one green only matters where we define a colour scheme which includes both brightgreen and green. If we define a colour scheme that only includes one of those two colours, then it doesn't really matter. There are 13 files where we define one or more colour schemes that include both brightgreen and green:
..and then changing those has knock-on effects on tests. I think we would need to update tests in the following 8 files:
Within the places where we have palettes that use both brightgreen and green there are then 2 cases within that:
When it comes to how to tackle this, I think there are 3 ways we can go:
Coming back to this with fresh eyes and going through all this code has made me realise a couple of additional things. The first one is relevant to this issue, and I'm going to chuck it into the mix here while we are still making decisions: If we're going to reduce the number of colours in the standard palette in the name of improving contrast/accessibility, and we think we should go down from 2 greens to 1 green, should we also go down from 2 yellows to 1 yellow? The second one is a complete tangent and we don't need to solve it here, so I'm going to move it to another issue, but while I was working through all this stuff I found that there's a lot of places where colour scales are different for no good reasons. e.g: There are places where we're defining a 3 point good --> bad scale that is brightgreen/yellow/red and other places where we're defining a 3 point good --> bad scale that is brightgreen/orange/red and there is not really any good reason why they are different. This feels like something we can clean up, but lets not do it here. Going down to a max 4 point scale would clean this up a lot though as there is way less wiggle room. I'm going to move the discussion of that elsewhere. |
|
I'm personally not sold on the idea of having two different colours palettes and supporting both via a URL parameter. I'd be in favour of just going ahead and making the changes presented in the PR description, possibly making brightgreen a little more bright and yellow a little less brown. We don't need to make things perfect, as long as it's not too disruptive nor confusing, any incremental change that improves legibility is a win in my opinion. |
This PR follows on from the discussion in #5497 . Its happening. Lets go.
Shields badges should be accessible by default. The first stated goal of shields is that badges should be
The objective of this PR is to improve the colour contrast (and hence accessibility and readability) of our badges. I'm going to do that by:
flat, which is the default and alsoplastic) to achieve greater contrast.There are a number of things I am explicitly not doing in this PR:
flat-squareorfor-the-badgestyles, which do not use a drop-shadow. The palette changes should increase contrast when using those styles, but in some cases those styles will still not achieve the target.Those are not problems we will never tackle, but changes to those things should be out of scope for this PR. Lets start off by making a small change that fixes the most common cases to start with. Those issues can be addressed at another time.
As it stands right now, this PR makes the following changes. As we tweak this PR, I will update this:
colours
build status
version
coverage
There are a number of things I have not done in this PR:
Obviously this change is high impact. Merging this will change the base palette for the first time in ~10 years, affect basically 100% of users and everyone is going to have opinions about it. I am confident we are making this change for the right reasons, but I'd like to make sure we have some kind of consensus on the outcome from the maintainer team and wider community before we merge this.
Maintainers: You can spin up a review app to review changes. I'd be particularly interested if anyone can spot any unintended consequences of this work that I may have missed as well as just feedback on the intended changes outlined in the tables above.