-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Improve label contrast ratios #1980
Conversation
2178b63
to
6149253
Compare
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.
This is great; thanks for taking this on!
Two blockers:
- this breaks a test
- from casual testing in Terminal.app, I'm seeing the contrast between bg and fg colors being worse (lower) than before 🤔
6149253
to
cd307c4
Compare
I've modified this PR to fix the test, changing the expected formatting to use the new 3-component color codes instead of the old palettized color codes. |
Hmm. The Travis build is failing with this:
https://travis-ci.com/apjanke/hub/builds/96120106?utm_medium=notification&utm_source=email Is that my fault? |
Can you give me an example of where the contrast looks worse in Terminal.app? I've been testing it against the silverstripe/silverstripe-admin repo that robbieaverill mentioned, and it looks better there. Here's hub 2.6.1 vs this PR for me: Offhand, I guess the issue could be that the old version is considering pure black and pure white as its candidates, and they should be added to the candidate color list in this PR in addition to the colors derived from the lightening/darkening algorithm? |
Pick between white and black depending on which one is the first to satisfy the contrast ratio of 7.0 (or 4.5 as fallback).
I've just pushed a simplified version. Please review. I'm afraid I've sent you on a wild goose chase with my initial specification for this feature. Although my comment was a near-accurate reflection of our server-side logic when picking a contrasting color candidate, I have failed to pick up the fact that we also add black The result in Terminal.app now matches how labels render in the web-interface (at least, in my observation): |
Oh well, it was a fun exercise! The new simplified version of the code looks good to me, and seems to work well when I've tested it against silverstripe-admin and a couple other repos, for both issues and prs. Looks good to me in both iTerm and Terminal.app. Colors do seem to match those displayed in the GitHub web interface in my testing too. Looks good. |
Fixes #1960.
Depends on and includes #1979.
This PR improves the contrast ratios of displayed label text by using a more sophisticated algorithm for picking foreground colors to go with the background issue colors.
I'm not sure I got the color candidate calculation right, because I'm not sure I understood the use of the terms "lightness" and "brightness" in this comment