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

Emoji Length Calculation Fix #92

Merged
merged 4 commits into from
Mar 13, 2019
Merged

Emoji Length Calculation Fix #92

merged 4 commits into from
Mar 13, 2019

Conversation

acupoftee
Copy link
Contributor

@acupoftee acupoftee commented Mar 12, 2019

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
Copy link
Contributor

Turbo87 commented Mar 12, 2019

@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
Contributor Author

Will do

package.json Outdated Show resolved Hide resolved
@Turbo87 Turbo87 added the bug Something isn't working label Mar 12, 2019
Copy link
Contributor

@Turbo87 Turbo87 left a comment

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
Copy link
Contributor

Turbo87 commented May 1, 2019

@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