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

Preview text too dark #3

Closed
corneliusroemer opened this issue Mar 19, 2024 · 10 comments · Fixed by #4
Closed

Preview text too dark #3

corneliusroemer opened this issue Mar 19, 2024 · 10 comments · Fixed by #4
Labels

Comments

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Mar 19, 2024

The text is quite a bit too dark to be readable in "preview" mode, i.e. when tabbing through keys:

image

I don't have any visual impairment so if I struggle it's likely impossible to read for people who don't have perfect vision.

In particular the dim blue can't be read against the black background.

It seems that your terminal shows the blue a bit differently:
image

@ynqa
Copy link
Owner

ynqa commented Mar 20, 2024

@corneliusroemer Thank you for your suggestion and even for the swift PR 👓

However, instead of changing the current defaults for themes including colors, I'm considering designing a config file that allows users to set their terminal themes on their side.

@daviddenton
Copy link

Firstly - JNV is great - so useful already - but I (and my pair) also struggled with the colour-scheme.

As a maintainer myself - I understand that you'll want time to do a proper job on the config file (which also sounds like a great idea btw). Would it be worth merging/release this in the meantime, which would be the best of both worlds? 😄

@ynqa
Copy link
Owner

ynqa commented Mar 20, 2024

@daviddenton cc @corneliusroemer

Thank you for using it ✌️ and I understand your point about wanting to temporarily change the color scheme for better visibility.

In that case, let's proceed with merging #4.

However, I have one comment. I would like to differentiate the colors for each UI component, so I want to separate the colors for readline prompt (=prefix) and JSON keys.

prefix_style: StyleBuilder::new().fgc(Color::Cyan).build(),
key_style: StyleBuilder::new().fgc(Color::Cyan).build(),

As a note, since most of the colors other than the Darkxxxx presets are already in use, it might be a good idea to specify using Color::Rgb { r: (), g: (), b: () }. If you have any suggestions for good colors, please let me know.

@nicolabeghin
Copy link

+1 for the color review, same issue - on a side note, excellent tool @ynqa !!! 💯

@daviddenton
Copy link

Splitting the colours for maximum tweaking is probably great from a colourblind perspective so I do think that would be a good option. No preferences on colour really as long as it's lighter than the dark blue 😉

@aodj
Copy link

aodj commented Mar 20, 2024

100% agree, I very nearly gave up on using jnv purely because of this issue.

@corneliusroemer
Copy link
Contributor Author

corneliusroemer commented Mar 20, 2024

A workaround is to change the colormap of your terminal's ANSI colors - this is actually a more sustainable change for readability. I'm using hyper and I can change the hex code for Blue (the color that's particularly hard to read) - that way that color becomes more readable for all tools and no longer needs to be avoided outright.

Terminal level customizability of ANSI colors is one of the reasons why using explicit rgb might harm accessibility @daviddenton If you are red-green color blind, the best thing to do is to remap the hex codes for the corresponding ANSI codes.

@ynqa ynqa closed this as completed in #4 Mar 21, 2024
@ynqa
Copy link
Owner

ynqa commented Mar 21, 2024

@corneliusroemer Thanks for your contributing 👍 I just released v0.1.2. Please check.

@corneliusroemer
Copy link
Contributor Author

Yah! This now works, excellent, thanks! @ynqa

@daviddenton
Copy link

yep. So much better! Thanks 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants