-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat(ext/console): convert colors to supported space #18549
base: main
Are you sure you want to change the base?
feat(ext/console): convert colors to supported space #18549
Conversation
e8657b4
to
48aa62d
Compare
ab6773e
to
cb0e1dd
Compare
d326722
to
1c6ef5d
Compare
b1f60f4
to
309d7d7
Compare
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.
Looks great @marvinhagemeister! I will wait for @kt3k's review before landing this, but overall LGTM
ext/console/02_console.js
Outdated
return `\x1b[${kind};2;${r};${g};${b}m`; | ||
} else if (supportLevel === COLOR_SUPPORT_256) { | ||
// Lower colors into 256 color space | ||
// Taken from https://github.com/Qix-/color-convert/blob/3f0e0d4e92e235796ccb17f6e85c72094a651f49/conversions.js |
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.
👍
Looks very nice feature to have! I'm in favor of the idea. @marvinhagemeister Can you write a few more integration type test cases which simulate various terminals/environments by setting |
@marvinhagemeister when you find the time, could you please rebase and add a test case mentioned above? |
309d7d7
to
6dbdd71
Compare
@kt3k @bartlomieju Tried adding some tests but I have trouble setting the environment variables before a test in rust. It seems like |
@marvinhagemeister by default tests in Rust are run on multiple threads - I'm pretty sure the tests are overriding env vars set by other tests. I suggest to do an integration test using |
@marvinhagemeister You can create a small script like the below: console.log("%chello", "color: #123456"); in itest!(test_color_support_term_256 {
envs: vec![("TERM".to_string(), "256".to_string())],
args: "run run/test_color_support.js",
output: "run/test_color_support.js.out",
}); (Note: The file names are arbitrary. You probably need several output file patterns.) |
🥰 |
Is it possible to provide an interface for applications to query color levels directly? Such as: |
Hi guys, I just wanted to check-in. Is this something you guys still want to pursue? This is a blocker for denoland/std#2602. It'd be good to have that one closed off 🙂 |
The default Terminal on macOS doesn't support 24bit True Colors, which lead to colors being rendered unexpectedly at times. This can be seen in the fresh initialization wizard where the expected colors should look like this (rendered in a True Color terminal):
With the changes in this PR we now check for the supported color space by the terminal and lower the color space if needed.
Before:
After:
Support table: