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

Add overline style and remove keyword, hsl, hsv, hwb and ansi color spaces #433

Merged
merged 11 commits into from Apr 22, 2021

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Apr 17, 2021

Update ansi-styles package

  • Add overline style
  • Remove keyword, hsl, hsv, hwb and ansi color spaces

#431

Fixes #360

Richienb added 2 commits Apr 17, 2021
…nsi` style

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb Richienb changed the title Add overline style, remove keyword, hsl, hsv, hwb and ansi style and stop backporting ansi256 to ansi on older terminals Add overline style, remove keyword, hsl, hsv, hwb and ansi style and stop downsampling to ansi on older terminals Apr 18, 2021
@Richienb
Copy link
Contributor Author

@Richienb Richienb commented Apr 18, 2021

Because we no longer downsample ansi is no longer active, level 1 is now effectively the same in function as level 2.

@Qix-
Copy link
Member

@Qix- Qix- commented Apr 18, 2021

Just curious, why aren't we downsampling anymore? That seems like a functional regression, given that there are still loads of terminals that only support the older 16 color palette.

@Richienb
Copy link
Contributor Author

@Richienb Richienb commented Apr 18, 2021

@sindresorhus first suggested to remove it: #300 (comment)

Then you in a seperate conversation, approved removal of ansi: #300 (comment)

@Qix-
Copy link
Member

@Qix- Qix- commented Apr 19, 2021

I don't see in there why we need to remove this functionality. I would say we should inline that from color-convert (you're free to copy over the ansi256.ansi16 function from conversions.js directly - not that the license would have prevented you anyway). Just mention it came from color-convert in a comment.

The concern about upscaling (#370) was valid, but downscaling is absolutely necessary to keep compatibility with people who do not have 256 support.

As for the comments about "removing" that - no, those say to remove color-convert and thus the majority of the conversion models, save for hex and rgb. I strongly believe we need to keep ansi256 as well, and be able to downsample to 16-colors if necessary.

cc @sindresorhus

  • If chalk is at level 1 and I give it an .rgb(), it should downsample.
  • If chalk is at level 2 and I give it an .rgb(), it should downsample.
  • If chalk is at level 1 and I give it an .ansi256(), it should downsample.
  • If chalk is at level 2 and I give it an .ansi256(), it should pass through.

If we remove downsampling, we're going to irreparably break a LOT of people.

@Richienb
Copy link
Contributor Author

@Richienb Richienb commented Apr 19, 2021

you're free to copy over the ansi256.ansi16 function from conversions.js directly - not that the license would have prevented you anyway

That function doesn't exist

@Qix-
Copy link
Member

@Qix- Qix- commented Apr 19, 2021

Right, you have to merge them - this is untested but should be close enough. Just needs a bit of polish for the Chalk conventions.

function ansi256To16(args) {
	if (args < 8) return 30 + args;
	if (args < 16) return 90 + (args - 8);

	let r, g, b;

	// Handle greyscale
	if (args >= 232) {
		const c = (args - 232) * 10 + 8;
		r = g = b = c / 255;
	} else {
		args -= 16;

		let rem;
		r = Math.floor(args / 36) / 5;
		g = Math.floor((rem = args % 36) / 6) / 5;
		b = (rem % 6) / 5;
	}

        const value = Math.max(r, g, b) * 2;

	if (value === 0) {
		return 30;
	}

	let ansi = 30
		+ ((Math.round(b) << 2)
		| (Math.round(g) << 1)
		| Math.round(r));

	if (value === 2) {
		ansi += 60;
	}

	return ansi;
}

Then just make a bunch of test cases as such:

// Just as an example.
const colorConvert = require('color-convert');
const assert = require('assert').strict.ok;
const {ansi256To16} = require('..');

// Start at 16 since color-convert hasn't been updated to
// correctly return the first 16 colors of ansi-256.
for (let i = 16; i < 256; i++) {
	assert(
		colorConvert.ansi256.ansi16(i) === ansi256To16(i)
	);
}

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb Richienb changed the title Add overline style, remove keyword, hsl, hsv, hwb and ansi style and stop downsampling to ansi on older terminals Add overline style, remove keyword, hsl, hsv and hwb style Apr 21, 2021
@Richienb
Copy link
Contributor Author

@Richienb Richienb commented Apr 21, 2021

Support for ansi has been added back in.

Richienb added 2 commits Apr 21, 2021
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Qix-
Qix- approved these changes Apr 21, 2021
Copy link
Member

@Qix- Qix- left a comment

I don't see anything glaringly wrong with the code; I'll let Sindre have the final look.

@Richienb Richienb changed the title Add overline style, remove keyword, hsl, hsv and hwb style Add overline style and remove keyword, hsl, hsv and hwb color spaces Apr 21, 2021
@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Apr 21, 2021

I'm confused. I understand preserving the downsampling, but shouldn't we still remove the explicit .ansi() method?

@Richienb
Copy link
Contributor Author

@Richienb Richienb commented Apr 21, 2021

IMO, there is no impact of leaving the function there so we might as well leave it for people wanting to use the ansi color space. However, I agree that its utility is quite rare but this argument could also be held for ansi256 since it is also a step down from rgb.

@Qix-
Copy link
Member

@Qix- Qix- commented Apr 21, 2021

People wanting to use the ansi color space should just use .red(), .blueBright(), etc as it's a 1:1 mapping. People doing programmatic generation of colors for whatever reason are going to be using 256 or truecolor - not 16 - as there is no logical rhyme or reason to the code (I always remember them as "traffic light, dark blue, pink, light blue, white" - completely arbitrary).

I agree with sindre here - no need for .ansi(). Chalk is a utility library, after all - you're always able to write escapes yourself with trivial syntax.

See also: https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb Richienb changed the title Add overline style and remove keyword, hsl, hsv and hwb color spaces Add overline style and remove keyword, hsl, hsv, hwb and ansi color spaces Apr 21, 2021
@Richienb
Copy link
Contributor Author

@Richienb Richienb commented Apr 21, 2021

Removed

@sindresorhus sindresorhus requested a review from Qix- Apr 21, 2021
index.d.ts Outdated Show resolved Hide resolved
Qix-
Qix- approved these changes Apr 22, 2021
Copy link
Member

@Qix- Qix- left a comment

LGTM

@sindresorhus sindresorhus merged commit 4cf2e40 into chalk:main Apr 22, 2021
2 checks passed
@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Apr 22, 2021

Nice work, @Richienb 👍

@Qix-
Copy link
Member

@Qix- Qix- commented Apr 22, 2021

Yes indeed :) Thanks @Richienb, super appreciated.

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

Successfully merging this pull request may close these issues.

3 participants