The Wayback Machine - https://web.archive.org/web/20230112122232/https://github.com/go-gitea/gitea/pull/16729
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

Jump to bottom

Compute proper foreground color for labels #16729

Merged
merged 4 commits into from Aug 25, 2021
Merged

Compute proper foreground color for labels #16729

merged 4 commits into from Aug 25, 2021

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Aug 19, 2021 •
edited

This properly ensures that the contrast ratio is maximised for any given color choice.

The comments explain the exact calculations done (you can check that I transcribed it correctly), but to fully clarify for review:

  1. Human eyes are better at detecting contrast than absolute differences in brightness. So, it's easier to see the difference between 0% and 10% brightness, than 90% and 100% brightness, even though it's the same absolute difference.
  2. As a result, sRGB scales the colours so that the 0-255 range for each component has more dark shades than bright shades.
  3. If you want to check the contrast between two colours, you need to undo this scaling.

@clarfonthey clarfonthey force-pushed the luminance branch 5 times, most recently from 6221115 to 2456418 Compare Aug 19, 2021
@codecov-commenter

This comment has been minimized.

Sign in to view
@silverwind
Copy link
Member

silverwind commented Aug 19, 2021 •
edited

Note there is a similar function in JS that should be made to match the backend implementation:

gitea/web_src/js/components/ContextPopup.vue

Lines 75 to 86 in c9bca8c

labels() {
return this.issue.labels.map((label) => {
const red = parseInt(label.color.substring(0, 2), 16);
const green = parseInt(label.color.substring(2, 4), 16);
const blue = parseInt(label.color.substring(4, 6), 16);
let color = '#ffffff';
if ((red * 0.299 + green * 0.587 + blue * 0.114) > 125) {
color = '#000000';
}
return {name: label.name, color: `#${label.color}`, textColor: color};
});
}

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 19, 2021

Good catch! I'll fix that this evening

@clarfonthey clarfonthey mentioned this pull request Aug 20, 2021
Open
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 20, 2021

Fixed!

@clarfonthey clarfonthey changed the title Actually compute proper foreground color for labels Compute proper foreground color for labels Aug 20, 2021
@jolheiser jolheiser added this to the 1.16.0 milestone Aug 24, 2021
@jolheiser
Copy link
Member

jolheiser commented Aug 24, 2021

I think this all looks great, would you be able to attach a couple example images?

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 24, 2021

Sure thing! I'll take some tonight with some sample colours.

jolheiser
jolheiser approved these changes Aug 24, 2021
View changes
Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

Disregard my other comment, for some reason I thought this was changing colors, rather it's just a different formula for choosing white/black text.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 24, 2021

I'll still take some tonight anyway, since it might be helpful for the changelog

@jolheiser jolheiser merged commit 29b971b into go-gitea:main Aug 25, 2021
2 checks passed
@clarfonthey clarfonthey deleted the luminance branch Aug 26, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath

@jolheiser jolheiser

Assignees
No one assigned
Labels
kind/ui lgtm/done
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@clarfonthey @codecov-commenter @silverwind @jolheiser @zeripath @GiteaBot