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

Fix Solid nickname color #277

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Conversation

neilalexander
Copy link
Contributor

This was accidentally changed to the action color in #232.

@casperstorm
Copy link
Member

casperstorm commented Mar 17, 2024

Hmm, this was because the nicknames in chat was action if solid was selected, but nicknames in sidemenu was text. I think we should consider accent for both instead. With text it all blends too much together.

Screenshot 2024-03-17 at 06 42 17

@neilalexander
Copy link
Contributor Author

This is probably one of those subjective/theme-specific things — to my eyes, the "blending together" was what made the nick list disappear into the periphery rather than stand out too much. For example, this stands out quite a lot:

Screenshot 2024-03-17 at 10 53 39

... whereas this felt more in-keeping to me with the rest of the theme:

Screenshot 2024-03-17 at 10 54 37

Accent colour might be OK, it doesn't look as though it's used by much else apart from text selection?

Another alternative is that we put it to text and add some permanent alpha to it, so that it's subtly different but not by much, for example at alpha 0.6:

Screenshot 2024-03-17 at 10 57 21

@casperstorm
Copy link
Member

casperstorm commented Mar 17, 2024

Colors are hard :)
Perhaps the solution is simply to do text.base for both nicklist and nicknames in buffer.
It will blend the most, which i assume people who use this option want.

I just think that colors: "solid" should provide the same color for nicklist and nicknames in buffer!

@neilalexander
Copy link
Contributor Author

Right now there are separate colors: options for the buffer and nick list, so they can be controlled separately. That seems like a nice option to keep? Then you can have colours in the buffer but not nick list (like I do), both or neither.

Presumably both use the nickname() function?

@casperstorm
Copy link
Member

Right now there are separate colors: options for the buffer and nick list, so they can be controlled separately. That seems like a nice option to keep? Then you can have colours in the buffer but not nick list (like I do), both or neither.

Presumably both use the nickname() function?

Correct, but initially the nickname color in the buffer was action and the colors in nicklist was text.
Then in main both is action, and this PR changes both to text. 😄

I am good with that, now that I've given it some thoughts.

Comment on lines 55 to 62
if seed.is_none() {
let mut color = theme.colors().text.base;
if transparent {
color = alpha(color, if dark_theme { 0.2 } else { 0.4 })
}
return Appearance { color: Some(color) };
}
Copy link
Member

Choose a reason for hiding this comment

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

What about:

    if seed.is_none() {
        let color = match transparent {
            true => theme.colors().text.med_alpha,
            false => theme.colors().text.base,
        };

        return Appearance { color: Some(color) };
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's much nicer and more compact, will do that!

@casperstorm
Copy link
Member

Could you also add a small line to CHANGELOG? Then it should be good to go.

@neilalexander
Copy link
Contributor Author

Done!

Copy link
Member

@casperstorm casperstorm left a comment

Choose a reason for hiding this comment

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

Thanks a bunch @neilalexander

@casperstorm casperstorm merged commit 544eb52 into squidowl:main Mar 18, 2024
1 check 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.

2 participants