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.

Sign up for GitHub

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

Jump to bottom

Add support for ansi colors and add .ansi256ToAnsi(), .rgbToAnsi() and .hexToAnsi() #71

Merged
merged 4 commits into from Apr 21, 2021
Merged

Add support for ansi colors and add .ansi256ToAnsi(), .rgbToAnsi() and .hexToAnsi() #71

merged 4 commits into from Apr 21, 2021

Conversation

Richienb
Copy link
Contributor

@Richienb
Add support for ansi colors and add .ansi256ToAnsi(), `.rgbToAnsi…
048a785 
…()` and `.hexToAnsi()`

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb Richienb mentioned this pull request Apr 20, 2021
Merged
@sindresorhus
Copy link
Member

@Qix- LGTY?

Qix-
Qix- requested changes Apr 20, 2021
View reviewed changes
Copy link
Member

@Qix- Qix- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Richienb and others added 3 commits April 21, 2021 12:01
@Richienb @Qix-
Update readme.md
fc3bd82 
Co-authored-by: Qix <Qix-@users.noreply.github.com>
@Richienb @Qix-
Update readme.md
6c6bb06 
Co-authored-by: Qix <Qix-@users.noreply.github.com>
@Richienb @Qix-
Update index.js
da564df 
Co-authored-by: Qix <Qix-@users.noreply.github.com>
@Richienb Richienb requested a review from Qix- April 21, 2021 00:01
@Richienb
Copy link
Contributor Author

The CI is yet to have cached a version of @babel/traverse released 44 minutes ago so it is currently failing as of now.

@Qix-
Copy link
Member

Qix- commented Apr 21, 2021 •
edited

Can we get some tests for ansi256ToAnsi specifically?

Otherwise, LGTM. Thanks for the heads up about the failing test.

@Richienb
Copy link
Contributor Author

Can we get some tests for ansi256ToAnsi specifically?

Otherwise, LGTM. Thanks for the heads up about the failing test.

Since this function is being added to color-convert (https://github.com/Qix-/color-convert/issues/92) it should really be tested there. That's why there aren't any tests here for rgb → ansi256.

@sindresorhus sindresorhus merged commit 2ceace1 into chalk:main Apr 21, 2021
@Qix-
Copy link
Member

Qix- commented Apr 21, 2021

I disagree, but okay. Chalk does not rely on color convert any longer so their testing concerns no longer overlap. If you're going to vendor code, you should include automated tests in the destination codebase, IMO.

@Richienb
Copy link
Contributor Author

If it's a direct copy-paste from the original codebase that is already tested, then IMO unit tests aren't nessessary. Now would be the time to add the short-circut to color-convert.

@sindresorhus
Copy link
Member

Sorry, I missed the comment about tests. I agree we should have some tests here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus approved these changes

@Qix- Qix- Awaiting requested review from Qix-

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Richienb @sindresorhus @Qix-