Skip to content

Emoji Length Calculation Fix - #92

Merged
Turbo87 merged 4 commits into
cli-table:masterfrom
acupoftee:master
Mar 13, 2019
Merged

Turbo87 merged 4 commits into
cli-table:masterfrom
acupoftee:master

Conversation

@acupoftee

@acupoftee acupoftee commented Mar 12, 2019 •

Copy link
Copy Markdown
Contributor

There was an issue with emoji padding inside table cells. For example, this was the result when the calculations used the string-width module:

Screen Shot 2019-03-12 at 9 52 57 AM

It turned out that stringWidth(s) in utils.js:13 returned the wrong length. The unicode length for the should be 1, however stringWidth("🇯🇵") returns 2.

This was corrected using the unicode-aware module Stringz. This PR is to use Stringz to return the correct emoji length and render the correct output as seen below:

Screen Shot 2019-03-12 at 9 52 37 AM

@Turbo87

Turbo87 commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

@acupoftee it seems that you used npm instead of yarn, which unintentionally created a package-lock.json file. could you remove that file from the first commit and instead update the yarn.lock file?

@acupoftee

Copy link
Copy Markdown
Contributor Author

Will do

Comment thread package.json Resolved
@Turbo87 Turbo87 added the bug Something isn't working label Mar 12, 2019

@Turbo87 Turbo87 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice work, @acupoftee!

@Turbo87
Turbo87 merged commit c1fb59c into cli-table:master Mar 13, 2019
Turbo87 added a commit to boneskull/cli-table3 that referenced this pull request May 1, 2019
@Turbo87 Turbo87 mentioned this pull request May 1, 2019
@Turbo87

Turbo87 commented May 1, 2019

Copy link
Copy Markdown
Contributor

@acupoftee I've had to revert this PR in #103 because we discovered that it was responsible for the test suite failing 😢

it seems that stringz does not handle CJK characters the same way and some of our tests are now failing. do you have time to have a look at that?

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants