Skip to content
This repository has been archived by the owner on May 4, 2024. It is now read-only.

Remove backgrounds from log #6

Closed
wants to merge 2 commits into from

Conversation

evanlucas
Copy link

This removes the background from the log heading as well as the following log levels:

  • verbose
  • http
  • error

See npm/npm#5202. It is much more appealing on dark terminals that do not have a solid black background.

@domenic
Copy link

domenic commented May 6, 2014

-1, I like how it looks as-is.

@isaacs
Copy link
Contributor

isaacs commented May 7, 2014

The background colors were added because they were requested by users who had a hard time seeing the colors on their default terminal setting.

You can say that it's their responsibility to configure your terminal. And maybe you're right. But I'd rather not teach people how to use their computers when it's easy to just accommodate their requests.

I'd like to see it more configurable. Also, I'd like to change npm to have a lot less output in general, so these colorful things would only be seen by folks who opt into them anyway.

@isaacs
Copy link
Contributor

isaacs commented May 7, 2014

Fwiw, I have a "dark terminal that does not have a solid black background", and I do prefer the black background behind the text. It make it seem more solid and contained in a way.

@evanlucas
Copy link
Author

Totally understand. Everyone has his or her preference (guess I'm more in the minority on this issue). As far as being configurable, would that be something that you would want to be in npmconf? Or maybe a function in here that could remove them? Be happy to help in any way on it. :]

@davidchambers
Copy link

npm install

npm is the only command-line utility I use frequently which specifies background colours. npm has a broad audience, though, so unattractive-but-guaranteed-to-be-legible is a defensible default.

+1 for configurability if it allows background colours to be disabled.

@isaacs
Copy link
Contributor

isaacs commented May 7, 2014

@davidchambers Since you're configuring the black background to be grey anyway, and if npm really is the only tool you use that specifies background colors, why not configure your terminal to not show background colors at all?

@davidchambers
Copy link

That's certainly an option. If I ever switch from MacVim to terminal vim, though, I'll want background colours for the status line, so it's not optimal.

@isaacs
Copy link
Contributor

isaacs commented May 7, 2014

You could compile vim for 256 colors, and just set the 16 basic ANSI background colors.

OMG so bikeshedding... ;)


log.style = {}
log.levels = {}
log.disp = {}
log.addLevel('silly', -Infinity, { inverse: true }, 'sill')
log.addLevel('verbose', 1000, { fg: 'blue', bg: 'black' }, 'verb')
log.addLevel('verbose', 1000, { fg: 'blue', }, 'verb')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing comma

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that's horrible. Fixed.

@davidchambers
Copy link

What's the status of this PR?

@davidchambers
Copy link

Any ❤️ for this PR?

@othiym23
Copy link
Contributor

@davidchambers I think, having read this thread, the best solution would be to come up with a way to make the background configurable, rather than just setting it outright. If you were to add that, this would be in a better place to land. Thanks for your patience!

@davidchambers
Copy link

Configurable via what mechanism, @othiym23?

@iarna
Copy link
Contributor

iarna commented Mar 16, 2016

@davidchambers, I think that's something a patcher would have to figure out. Currently configuration in this module is done via method calls, as npmlog is a singleton. Figuring out exactly what that'd look like is most of the work of preparing the PR.

I'm going to close this PR as we won't be accepting it as it is.

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

Successfully merging this pull request may close these issues.

None yet

6 participants