Skip to content
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

WIP Repaint the bike shed #9916

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP Repaint the bike shed #9916

wants to merge 3 commits into from

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Jan 21, 2024

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

legible: it should be as easy to read the metadata provided by a badge as it is to read body copy inside of a README file regardless of display resolution

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:

  1. Adjusting the palette of named/semantic colours we use to render 99% of our "official" badges (that is service badges on a named route).
  2. Using a more pronounced drop-shadow on badge styles that use a drop-shadow (flat, which is the default and also plastic) to achieve greater contrast.

There are a number of things I am explicitly not doing in this PR:

  • I'm not attempting to achieve LC-75 contrast when using the flat-square or for-the-badge styles, 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.
  • 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'm not making any changes to the handful of "official" badges that are using custom hex values outside of the standard palette, for whatever reason.

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

colour before after
brightgreen
green
yellowgreen
yellow
orange
red
blue
grey
lightgrey

build status

status before after
passing
failing
error

version

version before after
stable
pre-release

coverage

coverage before after
100%
95%
85%
75%
0%

There are a number of things I have not done in this PR:

  1. This PR makes changes to the badge-maker NPM package. I haven't bumped the package version or updated the changelog in this PR.
  2. We're going to need to do some communication around this change. As well as a pinned issue/discussion, one of the things I want to do is write a blog post. I'll write it before we merge this, but lets nail the code changes down first.

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.

@chris48s chris48s added core Server, BaseService, GitHub auth npm-package Badge generation and badge templates labels Jan 21, 2024
Copy link
Contributor

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against e7b26cc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@chris48s chris48s Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screenshot at 2024-01-22 14-25-38

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 just checking in @chris48s, is there anything I can do to help move this forward?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris48s how much burden would you say we'd have (codebase, runtime, issue maintenance, etc.) from any attempts to support both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 leave legacy as the default to start with
  • publicise that ?palette=accessible exists 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=accessible and you can opt out with ?palette=legacy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming: what does this look like for very pale colors such as aqua, beige, and lime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So those 3 would look like:

  • aqua
  • beige
  • lime

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.

@JoshuaKGoldberg
Copy link

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 😄

@PyvesB
Copy link
Member

PyvesB commented Jan 28, 2024

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.

@chris48s
Copy link
Member Author

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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@calebcartwright
Copy link
Member

At first glance, I'll note that "brightgreen" doesn't feel very bright anymore,

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)

@chris48s
Copy link
Member Author

While looking at another issue I noticed

color: '7cd958',

Just chucking a link to this here to remind myself when I get a chance to come back to this.

@chris48s
Copy link
Member Author

At first glance, I'll note that "brightgreen" doesn't feel very bright anymore,

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.

I think there might be a bit of an iron triangle here. There are several properties we want to optimise for:

  1. "brightgreen" should still look/feel bright
  2. "brightgreen" and "green" should be noticibly different
  3. there should be sufficient contrast between the text and background colours

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?

That may just be my screen and/or my eyes

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.

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

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.

@JoshuaKGoldberg
Copy link

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, 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. 🥲

@chris48s
Copy link
Member Author

chris48s commented May 7, 2024

Here's another thought on the green/brightgreen thing.

  • We can make the green and brightgreen more different, but doing so requires making the brightgreen darker
  • We can make the brightgreen more bright, but doing so makes the green and brightgreen harder to distinguish

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

  • green
  • yellowgreen
  • yellow
  • orange
  • red

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth npm-package Badge generation and badge templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants