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

Improve handling of invisible sequences in trim mode #33

Merged
merged 6 commits into from
Apr 2, 2019

Conversation

stroncium
Copy link
Contributor

Added tests from #32 and fixed code to pass them.

@sindresorhus
Copy link
Member

I merged #32, can you rebase and remove the .failing part from that test instead? I think it makes more sense to keep it as a separate test.

@sindresorhus sindresorhus changed the title improvement on handling of invisible sequences in trim mode Improve handling of invisible sequences in trim mode Mar 4, 2019
@sindresorhus
Copy link
Member

// @coreyfarrell

@coreyfarrell
Copy link
Contributor

coreyfarrell commented Mar 4, 2019

@sindresorhus I've just realized another question - should strings be wrapped before or after trimming? Currently wrapping is done before trimming which produces the following results:

// 3 spaces before / between / after
t.is(wrapAnsi('   foo   bar   ', 6), 'foo\nbar');
t.is(wrapAnsi('   foo   bar   ', 3), '\nfoo\n\nbar\n');

@stroncium
Copy link
Contributor Author

@coreyfarrell @sindresorhus It's not a question of trim first or not(trimming is not just a stage), it's a bug, and I guess part of it was introduced by my big patch. Anyhow, added test, fixed.

@coreyfarrell
Copy link
Contributor

@stroncium I just verified that all cliui tests now pass when wrap-ansi code is updated with this PR. Thanks!

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

This looks good to me, I've also verified the latest version allows cliui tests to pass.

@sindresorhus sindresorhus merged commit 67aa84d into chalk:master Apr 2, 2019
@sindresorhus
Copy link
Member

Thanks for fixing, @stroncium 👍

@kevva kevva mentioned this pull request Sep 30, 2019
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.

None yet

3 participants