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: Use brighter colors for better accessibility #4

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

corneliusroemer
Copy link
Contributor

@corneliusroemer corneliusroemer commented Mar 19, 2024

Resolves #3

Replace all Colors:DarkCOLOR with Colors:COLOR

The reason for the very dark colors before was the double darkening due to: a) background and b) dark version.

With this PR things are actually readable:
image

This is current main:
image

@rrfeng
Copy link

rrfeng commented Mar 20, 2024

I installed jnv using brew just now, but the color seems not changed?
image

@ynqa ynqa mentioned this pull request Mar 20, 2024
src/jnv.rs Outdated
@@ -74,16 +74,16 @@ impl Jnv {
history: Default::default(),
prefix: String::from("❯❯ "),
mask: Default::default(),
prefix_style: StyleBuilder::new().fgc(Color::DarkCyan).build(),
active_char_style: StyleBuilder::new().bgc(Color::DarkMagenta).build(),
prefix_style: StyleBuilder::new().fgc(Color::Cyan).build(),
Copy link
Owner

Choose a reason for hiding this comment

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

@corneliusroemer I would like to proceed with the merge, but please make a revision just regarding this #3 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! I'm not quite sure what exactly I should change here, can you make that change? You should have push rights to the branch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just made a change, turning one of the Cyans into Blue so that each color is used only once again

@ynqa
Copy link
Owner

ynqa commented Mar 20, 2024

@rrfeng This pull request is not yet merged. So the color theme is still v0.1.0 instead.

@rrfeng
Copy link

rrfeng commented Mar 20, 2024

@rrfeng This pull request is not yet merged. So the color theme is still v0.1.0 instead.

Oh I missed the status of this pull request :D

Cyan was used twice, use blue in one place instead (the one where readability is less crucial
@corneliusroemer
Copy link
Contributor Author

I've updated the PR as requested @ynqa

Copy link
Owner

@ynqa ynqa left a comment

Choose a reason for hiding this comment

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

LGTM! @corneliusroemer Thanks 👍

@ynqa ynqa merged commit 08a7d64 into ynqa:main Mar 21, 2024
6 checks passed
@corneliusroemer
Copy link
Contributor Author

Excellent! Now just a new release 😎

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.

Preview text too dark
3 participants