-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 non standard ANSI colors #2164
Conversation
We've had issues with changing colors in the past, so this will prob need a bit of time to review. That said, I haven't had any problems using those values with a toy project a couple years ago https://github.com/danielstjules/pho/blob/master/src/Console/ConsoleFormatter.php#L22-L35 |
Since we couldn't upgrade to chalk, we can prob copy values from ansi-styles: https://github.com/chalk/ansi-styles/blob/master/index.js#L27-L60 Note that we can't use ansi-styles directly since it would break compatibility with older version of NPM due to its use of tilde in package.json |
@jtangelder can you please rebase this onto |
Replaces the non standard 90-97 color range to their standard 30-37 range. This should fix colors from not showing up, like on Travis.
Rebased! |
@jtangelder Thanks! Someone (maybe me) should pull down this PR and see what mochify's output looks like. @mantoni are you interested in doing this? |
Sure. Looks like my computer is offline for the day due to some technical issues with my provider. Hopefully I'm able to check tomorrow. |
basically floored that nyan works w/ mochify |
So this solves #1200, apparently, but I can't reproduce a problem, so I'm having trouble understanding how this actually changes anything. Help? |
Hmm, this doesn't seem to fix #1200 when using iTerm and the official Solarized theme. I'm still seeing 'pass' text as the background color. Starting to look like a problem with Solarized though. It's using bright black as the background (base03 is the background for dark theme), which is the same as the 'pass' text that Mocha is using. I'd tend to assume that the value 'black' is going to be the background, and 'bright black' would show up on black (though maybe not well). Just a thought. |
@jtangelder Can you show an example of where Travis fails to show colors without this change? |
Can't reproduce anymore. But I think it still is a good idea to use the standard colors instead of the non-standard alternatives. |
In general I think this is a good idea. I'm not sure if this got merged in the shuffle. If anyone wants to double-check or look at the changelog, then I'll close it if it was. |
Closing for inactivity; unclear if issue reproducible |
Replaces the non standard 90-97 color range to their standard 30-37 range. This should fix colors from not showing up, like on Travis.