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

upgrade to version of cliui that handles ansi-colors #277

Merged
merged 3 commits into from
Oct 16, 2015
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Oct 13, 2015

I've updated the layout logic in cliui to take into account the ansi escape codes:

yargs/cliui#11
yargs/cliui#12

@nexdrew
Copy link
Member

nexdrew commented Oct 13, 2015

Found a small problem concerning option alignment when ANSI colors are used in a description for an option. Commit 9963cbd adds 2 tests to demonstrate this with a minimal example:

  1. Alignment without ANSI colors
  2. Alignment with ANSI colors

As is, the 1st test passes but the 2nd test fails. Not sure what the root problem is, will need to investigate later.

@nexdrew
Copy link
Member

nexdrew commented Oct 13, 2015

FYI the test comes from an example I put together in issue 251. Here's what the problem looks like:

screen shot 2015-10-13 at 12 54 14 am

@phated
Copy link

phated commented Oct 13, 2015

This is my before and after runs from gulp:

Before

screen shot 2015-10-13 at 3 17 23 pm

After

screen shot 2015-10-13 at 3 16 56 pm

As you can see, the wrapping colors now work correctly. I'm not sure what the result of the types column should look like though.

Thanks for all the work on this @bcoe!

@bcoe
Copy link
Member Author

bcoe commented Oct 16, 2015

@phated, @nexdrew sorry for being slow to update it's been a busy few weeks (I also apologize for how much of a bikeshed 🚲 🏠 this is at this point, want to make sure we solve this issue for good).

I've made some changes to how widths are calculated for ansi escape codes, @phated your right-hand-column should align [boolean], [count], etc, appropriately now, @nexdrew could you confirm that I've fixed the issues you were seeing?

@phated
Copy link

phated commented Oct 16, 2015

Looks good!

Screenshot of latest changes:
screen shot 2015-10-16 at 10 53 42 am

Thanks @bcoe for all the work you have been putting into this!

bcoe added a commit that referenced this pull request Oct 16, 2015
upgrade to version of cliui that handles ansi-colors
@bcoe bcoe merged commit 99e7944 into master Oct 16, 2015
@bcoe bcoe deleted the ansi-colors branch October 16, 2015 22:33
@nexdrew
Copy link
Member

nexdrew commented Oct 17, 2015

yargs@3.28.0 introduces some slight problems for yargonaut (when combining colors with figlet fonts), but that's yargonaut's problem. Everything else looks good. 👍

@bcoe
Copy link
Member Author

bcoe commented Oct 18, 2015

@nexdrew your sure this is a yargonaut bug, and not something we can fix in cliui or yargs (while we're tearing these libraries apart any ways).

@nexdrew
Copy link
Member

nexdrew commented Oct 18, 2015

@bcoe Well, I think cliui might be unnecessarily trimming leading whitespace when ANSI colors are used. See yargonaut issue 7 for example.

Was planning to look into it and offer a fix, if necessary. Unfortunately I am AFK at the moment and won't have a chance to look til later tonight or tomorrow.

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