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

Color issue with "Install ... to use ..." feature #533

Open
Kocal opened this Issue Mar 5, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@Kocal
Copy link
Contributor

Kocal commented Mar 5, 2019

Ref: #520

If we have a light theme, the word Install is unreadable:
selection_301

It works fine with dark theme:
selection_302

It would be nice if it take the same color than Running webpack ..., yarn dev-, to use... which is the default color.

I quickly checked the source code on GitHub but I wasn't able to find where the problem apprears.

Thanks!

@Lyrkan

This comment has been minimized.

Copy link
Collaborator

Lyrkan commented Mar 5, 2019

I think that's caused by pretty-error, the following snippet should give you the same result:

const PrettyError = require('pretty-error');
const chalk = require('chalk');

const pe = new PrettyError();
console.log(pe.render(
  `Install ${chalk.green('webpack-notifier')} to use ${chalk.green('enableBuildNotifications()')}`
));

And here is an issue that's probably related: AriaMinaei/pretty-error#7

@Kocal

This comment has been minimized.

Copy link
Contributor Author

Kocal commented Mar 5, 2019

Yep that's a pretty-error issue! :)
selection_304

I found a workaround by using chalk.reset:

const PrettyError = require('pretty-error');
const chalk = require('chalk');

const pe = new PrettyError();
console.log(pe.render(
-  `Install ${chalk.green('webpack-notifier')} to use ${chalk.green('enableBuildNotifications()')}`
+  chalk.reset(`Install ${chalk.green('webpack-notifier')} to use ${chalk.green('enableBuildNotifications()')}`)
));

selection_306

That's not ideal but it do the job. 🤔

(But the issue should be fixed on pretty-error ofc)

@Lyrkan

This comment has been minimized.

Copy link
Collaborator

Lyrkan commented Mar 5, 2019

The issue is that we can't really do it in some cases...

That module is called in a global try/catch here (also a bit below but that one shouldn't be a problem) and as you can see it takes the exception as a parameter.

Since we don't know what will cause an exception we can't assume that it will always be a string and use chalk.reset on it.

Of course we could check if (typeof error === 'string') before, but that would only fix it for the few cases where it's a string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.