search

chalk / wrap-ansi

Wordwrap a string with ANSI escape codes
MIT License
120 stars 25 forks source link

Add option to not remove whitespace - fixes #9 #17

Closed SamVerschueren closed 7 years ago

SamVerschueren commented 7 years ago

This PR tries to solve issue #9 where wrap-ansi removes leading and trailing whitespace by default.

Although the tests succeed, and log-update works again as expected, I don't think it's really the expected output that we get in the tests.

When I highlight the output, this is what I see

screen shot 2017-06-13 at 22 17 58

I don't think I should have those extra whitespace character (1 in green, 1 in red). I believe they should be added to the next line instead. So I will have a look at how I can solve that.

But please, feedback is more than appreciated! For instance, should we have extend the trim option to also accept leading, trailing so that users can choose to only remove trailing or only remove leading whitespaces? log-update for instance should have enough by only trimming the trailing whitespace.

// @SBoudrias @sindresorhus @Qix- @bcoe

SamVerschueren commented 7 years ago

Found a way to have it like this

screen shot 2017-06-13 at 22 49 46

But the solution is really dirty IMO so I want to explore other options first. But to be sure, is this what it should look like?

Note, it is a highlighted screenshot to show the start and end of the ANSI escape codes. This is the real output.

screen shot 2017-06-13 at 22 53 16
SamVerschueren commented 7 years ago

I came a bit further in solving this with PR #19.

keevitaja commented 7 years ago

Thank you. Nice pull request! I was about to write my own word-wrapper until saw this.

sindresorhus commented 7 years ago

I came a bit further in solving this with PR https://github.com/chalk/wrap-ansi/pull/19.

Now that #19 is merged, is this PR still up to date?

SamVerschueren commented 7 years ago

I think it is. But let me verify when I'm on my computer. It's been a while since the PR :).

SamVerschueren commented 7 years ago

Still have some additional work for this as the current solution does not make the cut at the correct position. It should look like this right?

screen shot 2017-06-13 at 22 53 16

Then I can adjust the test and do the additional work.

sindresorhus commented 7 years ago

It should look like this right?

Yes

SamVerschueren commented 7 years ago

Fixed it with the last commit. I believe this should do it.

sindresorhus commented 7 years ago

@SamVerschueren Sorry, you have a merge conflict after #20.

SamVerschueren commented 7 years ago

No problem, should be fixed now.

sindresorhus commented 7 years ago

Awesome! Hopefully we can finally get log-update fixed now :)

SamVerschueren commented 7 years ago

Let me check that :).