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 diff colors #1648

Merged
merged 1 commit into from
May 9, 2015
Merged

Fix diff colors #1648

merged 1 commit into from
May 9, 2015

Conversation

nylen
Copy link
Contributor

@nylen nylen commented Apr 8, 2015

This fixes diffs being unnecessarily hard to read:

Before
screen shot 2015-04-08 at 2 19 29 pm

After
screen shot 2015-04-08 at 2 22 03 pm

Others are experiencing the same issue with standard terminal color schemes too: #1200 (comment)

@vyp
Copy link

vyp commented May 3, 2015

This might be my own fault, but would be it possible to keep the background colors untouched? As in, have 32 instead of '32;40' and 31 instead of '31;40'? Would that look ugly for you though?

Because for me, the first command's output is with values '32;40' and '31;40', but the second is with values of just 32 and 31, and clearly the second output looks better: http://i.imgur.com/rwnImmI.png

@vyp
Copy link

vyp commented May 3, 2015

Actually, if you want to explicity set the background, then it should be 49 I think, not 40. (i.e. '32;49' and '31;49') How does 49 look for you? I think in theory it should look the same for your after screenshot. For me it looks normal, white background as in the second output of my screenshot above.

@nylen
Copy link
Contributor Author

nylen commented May 4, 2015

I'm fine with trying out additional variations if someone from @mochajs can weigh in first. For now, it seems like no one is watching the PRs here.

cc @a8m @boneskull @danielstjules @dasilvacontin @jbnicolai @pkozlowski-opensource @travisjeffery

@vyp
Copy link

vyp commented May 4, 2015

Okay but 49 I think is the "default background color", so I think it should be that, if it indeed doesn't look ugly that is. (And 40 is "black".)

@danielstjules
Copy link
Contributor

I think the contrast will vary quite a bit depending on your terminal color profile. However, I do prefer the changes applied by this PR as it bring it closer to standard diff tools (svn diff, git diff)

@vyp
Copy link

vyp commented May 4, 2015

It will obviously, but 40 being "black" (which is probably a dark color for most themes) doesn't play nice with light-background themes, although it does with dark-background themes. But 49 I think should play nice with both? Have you checked if 49 looks different than 40 for your own case @danielstjules?

@vyp
Copy link

vyp commented May 4, 2015

Source for that: https://web.archive.org/web/20150319145101/http://misc.flogisoft.com/bash/tip_colors_and_formatting#background

Although I'm not sure how credible that page is (but it ranks highly in google search results if that counts at all).

@nylen nylen force-pushed the fix/diff-colors branch from 402b350 to 6ee3fb5 Compare May 5, 2015 05:59
@nylen
Copy link
Contributor Author

nylen commented May 5, 2015

I don't care much about explicitly setting the background color. It looks nicer for me that way but that's just OS X Terminal.app being weird, and I don't want to break things for people with light terminal themes.

Updated the PR accordingly - @danielstjules I think this is good to go now. Also, could you take a look at #1647? It's another PR intended to make mocha's diff output consistent with standard tools.

@vyp
Copy link

vyp commented May 5, 2015

@nylen No worries. 👍

@nylen
Copy link
Contributor Author

nylen commented May 7, 2015

@vyp @danielstjules is this library still maintained??

@danielstjules
Copy link
Contributor

I'm happy with this. +1 from me. Just wanna hear from one more contributor.

@mochajs/mocha

@boneskull
Copy link
Contributor

I'll take a gander at this later.

boneskull added a commit that referenced this pull request May 9, 2015
@boneskull boneskull merged commit 789ac58 into mochajs:master May 9, 2015
@boneskull
Copy link
Contributor

@nylen thanks!

@FelikZ
Copy link

FelikZ commented Jul 22, 2015

This does not Fixes #802 for me. Because 90 is a bg color in Solarized theme and it still used on titles:

exports.colors = {
    'pass': 90
    , 'error stack': 90
    , 'fast': 90
    , 'light': 90
    , 'diff gutter': 90
}; 

I suggest to replace them to 94 which is looks really ok both in Solarazied and default terminal color scheme.

Solarazed:
solarized

Normal:
normal_colors

@legomind
Copy link

legomind commented Oct 7, 2015

Any progress on this?
When using solarized, error stacks are invisible.

screenshot12

@nmschulte-aviture
Copy link

+1 for this issue -- I'm using the popular solarized scheme with rxvt-unicode, via solarized/xresources. My results are the same as @legomind's: unusable.

@dasilvacontin
Copy link
Contributor

Same here. I'm interested in seeing this fixed, but I don't have much time lately...

@nmschulte-aviture
Copy link

So, 1) I just realized this isn't an issue, but a pull-request. 2) I updated to use the latest version and the error does not exist. @dasilvacontin, @legomind

@nylen nylen deleted the fix/diff-colors branch June 25, 2018 02:33
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.

Please change color scheme Much of the color output is invisible with solarized terminal theme
8 participants