Skip to content

Change color used for standard output text.#23

Closed
boushley wants to merge 1 commit intoexpressjs:masterfrom
boushley:master
Closed

Change color used for standard output text.#23
boushley wants to merge 1 commit intoexpressjs:masterfrom
boushley:master

Conversation

@boushley
Copy link
Copy Markdown
Contributor

This alleviates an invisible text problem in some themes.

Many themes have trouble with the Grey ANSI color 90 that is being used in dev output.

I'm seeing this in Solarized Dark, but it is a problem in other themes as well.

Changing from 1b[90m to 1b[92m fixed the problem for me.

Last year Bower made a similar change for the same reason.

@dougwilson
Copy link
Copy Markdown
Contributor

So what is color 92 supposed to look like? On Windows cmd.exe it shows up as a bright green.

@dougwilson
Copy link
Copy Markdown
Contributor

To me, it doesn't make sense to go from grey to bright green. Can we just make it like white instead?

@boushley
Copy link
Copy Markdown
Contributor Author

Looks like the green is the default color. Solarized shows it as a grey, sorry about that.

It'd probably be best to go with something more standard like 37 for white or to reset with 0 to the default terminal colors.

What's your thoughts.

92 Pics:
defaultcolor
solarized

37 (white) Pics:
solarized37
default37

0 (reset) Pics:
default-reset
solarized-reset

@boushley
Copy link
Copy Markdown
Contributor Author

I agree, the bright green doesn't make sense for most fonts. I would lean towards just doing a 0 for reset, but if you want to do 37 for white, that makes sense too.

@dougwilson
Copy link
Copy Markdown
Contributor

Yea, let's just go with the explicit white for now I suppose :) So at the very least the colors will work for people's themes.

@boushley
Copy link
Copy Markdown
Contributor Author

I've changed to explicit white (37), although I think default terminal text color (0) would be the most robust.

For reference here is what the 90 grey looked like before:

solarized-90

@dougwilson
Copy link
Copy Markdown
Contributor

For reference here is what the 90 grey looked like before:

Oh wow, that is terrible.

@boushley
Copy link
Copy Markdown
Contributor Author

Using a light color scheme can make 37 almost as bad. Here is what the explicit white (37) looks like on machine with solarized light (you'll get similar problems with any light color scheme).

solarized-light-37

And here is what solarized light looks like with color reset (0):
solarized-light-0

If you look at the other screens I attached for color reset (0) I think this is the solution that looks the best. Now if someone had a green, red, yellow or cyan background they would have issues with the color of the numbers, but I have to guess those are far less common issues than general light vs dark background color.

@dougwilson
Copy link
Copy Markdown
Contributor

OK. Really, in the end, i.m.o. it's the theme's job to make the colors not conflict, otherwise no matter what color you choose it'll conflict with some theme.

@boushley
Copy link
Copy Markdown
Contributor Author

I agree for the most part. The problem lies in being overly specific with what color you want text to be. The original problem was forcing a dark gray text, which conflicts with any dark background color. So the theme author has to either ignore what the terminal is asking for, or allow conflicts like this to occur.

In general a color reset like 0 is going to always display a good contrasting color unless the theme is totally busted.

I can just use my own modifications on morgan if you don't want to land this. Solarized has an issue around this, but I don't think it's going anywhere.

@dougwilson
Copy link
Copy Markdown
Contributor

I can just use my own modifications on morgan if you don't want to land this. Solarized has an issue around this, but I don't think it's going anywhere.

No, I'll land it either as white or reset. I don't like how the current one is not even using a standard ANSI color code anyway.

This alleviates an invisible text problem in some themes.

This should resolve: #22
@boushley
Copy link
Copy Markdown
Contributor Author

Alright, I've changed to the color reset.

@dougwilson
Copy link
Copy Markdown
Contributor

Your change has been released in version 1.2.0

@expressjs expressjs locked and limited conversation to collaborators Jul 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants