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

[BUG] Update notice has black background even on white terminals #3091

Closed
fregante opened this issue Apr 16, 2021 · 7 comments
Closed

[BUG] Update notice has black background even on white terminals #3091

fregante opened this issue Apr 16, 2021 · 7 comments
Assignees
Labels
Bug thing that needs fixing config:display Issues dealing with display of data to terminal Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@fregante
Copy link
Contributor

fregante commented Apr 16, 2021

Self-explanatory 👇
Screen Shot 3

My terminal theme has a white background (macOS default) but npm unnecessarily shows the update notice with a black color.

Environment:

  • OS: macOS 11.2.3
  • Node: 15.12.0
  • npm: 7.6.3
  • Fish Shell 3.2.1

I don't know if this has already been addressed since it will only appear when I'm not running the latest version.

@fregante fregante added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Apr 16, 2021
@nlf nlf added Priority 2 secondary priority issue and removed Needs Triage needs review for next steps labels Apr 16, 2021
@nickfryar
Copy link

The background for the update notifier is being set here:

const messagec = !useColor ? message : chalk.bgBlack.white(message)

And per chalk/chalk#102 it appears there's no way to detect background color in the terminal.

@fregante
Copy link
Contributor Author

I think the point is to not use bgBlack nor white at all. They should just be the default color as defined by the terminal. It makes no sense to set a black background on a black terminal in the first place. Same goes for a white text on a black terminal.

@nickfryar
Copy link

Does this extend to all cases of bgBlack? I found 8 instances of this with various colors in the cli. If it's just a case of removing bgBlack / bgBlack + white, I can open a PR for this. I'm still new to this repo though, so some direction on this would be appreciated!

@fregante
Copy link
Contributor Author

fregante commented Apr 17, 2021

It appears that in most cases it's trying to highlight a line. I think the correct solution for that is to use the inverse function instead, but I'm not familiar with either project to tell you for sure.

@nickfryar
Copy link

Chalk supports inverse, but ansicolors does not. All but one case of bgBlack use chalk, so that one exception can likely just be switched from ansicolors to chalk.

However, npmlog handles the highlighting differently, so the npm notice text in your screenshot above would still appear with a black background. What is the typical process for making changes in dependencies for the cli, such as this?

@wraithgar
Copy link
Member

this is now half fixed as of v7.20.0.

npmlog itself having forced black backgrounds for certain levels is going to be a separate fix.

@wraithgar wraithgar self-assigned this Jul 15, 2021
@lukekarrys lukekarrys assigned lukekarrys and unassigned wraithgar Oct 11, 2021
@lukekarrys lukekarrys removed their assignment Jul 11, 2022
@lukekarrys lukekarrys self-assigned this May 11, 2024
@lukekarrys lukekarrys added the config:display Issues dealing with display of data to terminal label May 11, 2024
@lukekarrys
Copy link
Contributor

This was fixed by 9216d59 and included in npm@10.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing config:display Issues dealing with display of data to terminal Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

5 participants