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

Diff colors render diff text unreadable #493

Closed
ravinggenius opened this issue Nov 24, 2015 · 13 comments
Assignees
Labels
bug
Milestone

Comments

@ravinggenius
Copy link

@ravinggenius ravinggenius commented Nov 24, 2015

When displaying a diff, the background colors are set. On my system (I'm using Solarized Light), the text color is already a not dark grey, so the text in the diff (arguably the most important part) is very difficult to read. Please explicitly set the foreground color anytime the background color is set. I've included a screenshot to demonstrate:

screen shot 2015-11-17 at 21 10 52

@geek geek added the bug label Nov 25, 2015
@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Nov 25, 2015

This looks like an easy one to fix, maybe a contributor could do it ?

@alobodig

This comment has been minimized.

Copy link
Contributor

@alobodig alobodig commented Nov 30, 2015

For what it's worth, I use a dark color scheme and it's also difficult to read. I think setting the text to bright white whenever the background is set would fix readability in both situations.
screen shot 2015-11-30 at 1 58 44 pm

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 3, 2015

What text color would everyone prefer? Is black okay? I'm new to contributing in general, but I would like to take a stab at this if that's okay with everyone else. Might take me a couple of days because of work being busy.

@alobodig

This comment has been minimized.

Copy link
Contributor

@alobodig alobodig commented Dec 4, 2015

Black is okay with me.

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 7, 2015

Working on it now...sorry it took me a while to get back to you. I ended up spending my weekend with an old friend (the flu).

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 7, 2015

Does this look okay?
actual-expected

@ravinggenius

This comment has been minimized.

Copy link
Author

@ravinggenius ravinggenius commented Dec 7, 2015

I think it's better, but the actual is still difficult to read. expected is okay though.

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 7, 2015

@ravinggenius, you're right. I think white looks better on the red...what do you think?
fail

@ravinggenius

This comment has been minimized.

Copy link
Author

@ravinggenius ravinggenius commented Dec 8, 2015

White on red is better. Can you test this with a light-colored terminal theme. I'm using Solarized Light in the screenshot above.

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 9, 2015

This is what it looks like on a white background:
lightbg

Unfortunately, on my Linux box at home (tested with a Windows PC at work), the white text for the red box reverts to whatever my console is set to, so it looks like this:
actual

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 9, 2015

I guess using ANSI escape code 37 (30+7) in place of 255 works for xterm, cmd, & cygwin. I'll update the pull request with the commit either tonight or tomorrow. Hopefully it works in OSX, but I have no way to test.

@alobodig

This comment has been minimized.

Copy link
Contributor

@alobodig alobodig commented Dec 9, 2015

Looks great in Terminal.app.

screen shot 2015-12-09 at 10 14 45 am

@thephilip

This comment has been minimized.

Copy link
Contributor

@thephilip thephilip commented Dec 9, 2015

Thanks for testing, @alobodig .

@geek geek added this to the 8.0.1 milestone Dec 11, 2015
@geek geek self-assigned this Dec 11, 2015
@geek geek closed this Dec 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.