Skip to content

Conversation

@ApocDev
Copy link

@ApocDev ApocDev commented Aug 4, 2025

This pull request improves the text contrast for highlighted rows in dark mode.

It also simplifies the color scheme setup by not using a magic array.

image

Before:
image

@ApocDev ApocDev requested a review from shpaass as a code owner August 4, 2025 13:54
@shpaass
Copy link
Owner

shpaass commented Aug 4, 2025

To facilitate the review, could you please post the before-after comparison?

@ApocDev
Copy link
Author

ApocDev commented Aug 4, 2025

To facilitate the review, could you please post the before-after comparison?

Added the before picture. (Note: the checkboxes are a separate PR I'll be adding shortly, so ignore them for now)

@Dorus
Copy link
Collaborator

Dorus commented Aug 4, 2025

This was already broken before, but possibly you could sweep this up too: The yellow exclamation marker is also quite dark and unreadable in dark mode:

afbeelding possibly make it lighter?

Copy link
Collaborator

@DaleStan DaleStan left a comment

Choose a reason for hiding this comment

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

I thought the use of different foreground colors was intentional (@veger, @shihan42 ?), but I'm not opposed to consolidating them.

@ApocDev
Copy link
Author

ApocDev commented Aug 4, 2025

I thought the use of different foreground colors was intentional (@veger, @shihan42 ?), but I'm not opposed to consolidating them.

It may have been intentional, but it's also incredibly difficult to read. Reading the blue is nearly impossible in some cases.

Typically, text vs background should have a contrast ratio of at least 4.5:1. The blue alone is only 1.77:1.

That said, in a lot of dark-mode schemes, text is just various shades of gray, and you utilize the background color for more "broad" information, especially when it's "just" text, and not things like headers, buttons, etc.

@Dorus
Copy link
Collaborator

Dorus commented Aug 4, 2025

I did like the old foreground color, but contrast is more important to me than the nice look, so i like the new option more. We can always go back to different foreground colors with better contrast later.

I've updated this to split out icon colors entirely, and modified it slightly.

Yeah nice, my numbers where just a suggestion, my feel for colors is terrible overall :)

@veger
Copy link
Collaborator

veger commented Aug 4, 2025

I thought the use of different foreground colors was intentional

There is some maths going on with the colors, that is why they are always in 'pairs' of 4. For example:

using (gui.EnterGroup(padding ?? DefaultButtonPadding, active ? color + 2 : color + 3)) {

(But there are more places)

So simply changing the meaning of the 4 colors might have some side effects, if we are not careful and check all scenarios.

I cannot remember the exact reasoning behind them anymore, too long ago... 😄

@shihan42
Copy link
Collaborator

shihan42 commented Aug 5, 2025

Looks like a good idea. I must confess I didn't really care for the dark mode colors back when I did the row highlighting.
So go for it, make it look better!

And yes, the strange color array thingamabob should really be refactored. I was to give it a try, but then RL happened. Feel free to give it a make over.

@ApocDev
Copy link
Author

ApocDev commented Aug 5, 2025

Looks like a good idea. I must confess I didn't really care for the dark mode colors back when I did the row highlighting. So go for it, make it look better!

And yes, the strange color array thingamabob should really be refactored. I was to give it a try, but then RL happened. Feel free to give it a make over.

I'll look into this as a separate PR. It might be part of some changes to the input system that I'm working on. (The current one is... "ok" at best)

@shpaass shpaass force-pushed the fix/dark-mode-readability branch from e776b87 to f3ccc8b Compare August 8, 2025 09:53
@shpaass
Copy link
Owner

shpaass commented Aug 8, 2025

Squashed the changelog commit into the first commit.
Moved the reminder in changelog below the general introduction.
Reworded the commit subjects so they fit the Github 70-character limit.
Squashed two last commits together because the latter fixes the former, from what I understand.

@shpaass shpaass merged commit ea8693a into shpaass:master Aug 9, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants