-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 fix #3607
Color fix #3607
Conversation
Test timeouts 😞 |
I was going to merge this PR, but was a little unclear on the "Fix prettier on windows." commit. |
It's not relate to the Bug, but the old prettier script was not working on windows. Command too long error. I found this solution on Facebook react project. I can remove that commit and open a separate pr if you want. |
Removed prettier commit and opened another PR #3635 |
@@ -27,6 +27,12 @@ const tty = require('tty'); | |||
type Row = Array<string>; | |||
type InquirerResponses<K, T> = {[key: K]: Array<T>}; | |||
|
|||
// fixes bold on windows | |||
// $FlowFixMe TERM is not a string? | |||
if (process.platform === 'win32' && !/^xterm/i.test(process.env.TERM)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does flow
still complain if you do
if (process.platform === 'win32' && process.env.TERM && !/^xterm/i.test(process.env.TERM)) {
And, how about using startsWith
instead of the regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can test it, this was copied from /chalk/chalk/blob/v1.1.1/index.js#L8
startsWith is not case insensitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I missed the i
.
It's better if we can get rid of $FlowFixMe
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
@@ -27,6 +27,11 @@ const tty = require('tty'); | |||
type Row = Array<string>; | |||
type InquirerResponses<K, T> = {[key: K]: Array<T>}; | |||
|
|||
// fixes bold on windows | |||
if (process.platform === 'win32' && process.env.TERM && !/^xterm/i.test(process.env.TERM)) { | |||
chalk.styles.bold.close += '\u001b[m'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be submitted upstream to the chalk
project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See chalk/chalk#145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, sounds like we can remove this hack in the future, then.
Summary
This PR resets the console style after using bold, because the new windows console does not support the ansi escape codes ^[[21m and ^[[22m. This would fix #2724
Test plan
Only one test case had to be fixed. :)