Skip to content

Compute proper foreground color for labels - #16729

Merged
jolheiser merged 4 commits into
go-gitea:mainfrom
clarfonthey:luminance
Aug 25, 2021
Merged

jolheiser merged 4 commits into
go-gitea:mainfrom
clarfonthey:luminance

Conversation

@clarfonthey

@clarfonthey clarfonthey commented Aug 19, 2021 •

Copy link
Copy Markdown
Contributor

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 August 19, 2021 04:28
@codecov-commenter

This comment has been minimized.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 19, 2021
@silverwind

silverwind commented Aug 19, 2021 •

Copy link
Copy Markdown
Member

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

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
Copy Markdown
Contributor Author

Good catch! I'll fix that this evening

@clarfonthey

Copy link
Copy Markdown
Contributor Author

Fixed!

@clarfonthey clarfonthey changed the title Actually compute proper foreground color for labels Compute proper foreground color for labels Aug 20, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 21, 2021
@jolheiser jolheiser added the topic/ui Change the appearance of the Gitea UI label Aug 24, 2021
@jolheiser jolheiser added this to the 1.16.0 milestone Aug 24, 2021
@jolheiser

Copy link
Copy Markdown
Member

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

@clarfonthey

Copy link
Copy Markdown
Contributor Author

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

@jolheiser jolheiser left a comment

Copy link
Copy Markdown
Member

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.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 24, 2021
@clarfonthey

Copy link
Copy Markdown
Contributor Author

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
@clarfonthey
clarfonthey deleted the luminance branch August 26, 2021 17:05
@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.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants