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

Add support for surrogate pairs and full width characters #20

Merged
merged 7 commits into from
Jul 22, 2017

Conversation

kevva
Copy link
Contributor

@kevva kevva commented Jul 21, 2017

Also added some small code tweaks for consistency :).

Fixes #10 and fixes #11.

index.js Outdated
if (visible + charLength <= cols) {
rows[rows.length - 1] += x;
} else {
rows.push(x);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe we should set visible = 0 here too since it's reset.

index.js Outdated

if (visible >= cols && i < word.length - 1) {
if (visible >= cols && i < arr.length - 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this should probably be visible === cols to avoid duplicating the if statement above.

index.js Outdated
for (let i = 0, word; (word = words[i]) !== undefined; i++) {
for (const item of Array.from(words).entries()) {
const i = item[0];
const x = item[1];
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a more descriptive variable than x here? Since the scope is large, it makes it hard to read the code.


// For each newline, invoke the method separately
module.exports = (str, cols, opts) => {
return String(str)
.normalize()
Copy link
Member

Choose a reason for hiding this comment

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

Why normalize here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To normalize unicode characters that creates the same characters but with different code points and possibly different lengths.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense

Copy link
Contributor Author

@kevva kevva Jul 21, 2017

Choose a reason for hiding this comment

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

Like this which both produces ñ:

'\xF1' === 'n\u0303'
//=> false

test.js Outdated
t.is(m('a\ud83c\ude00bc', 2, {hard: true}), 'a\n\ud83c\ude00\nbc');
t.is(m('a\ud83c\ude00bc\ud83c\ude00d\ud83c\ude00', 2, {hard: true}), 'a\n\ud83c\ude00\nbc\n\ud83c\ude00\nd\n\ud83c\ude00');
Copy link
Member

Choose a reason for hiding this comment

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

Can you use uppercase escapes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

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

Think it looks good! Some small remarks.

index.js Outdated

if (ESCAPES.indexOf(y) !== -1) {
const code = parseFloat(/\d[^m]*/.exec(pre.slice(j, j + 4)));
if (ESCAPES.indexOf(x) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make ESCAPES a Set and use ESCAPES.has(x) here?

index.js Outdated
const y = pre[j];
for (const item of Array.from(pre).entries()) {
const i = item[0];
const x = item[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call x char or something (like in the wrapWord function)? A variable named x doesn't say much.

@sindresorhus sindresorhus merged commit f16f943 into chalk:master Jul 22, 2017
@sindresorhus
Copy link
Member

I really appreciate you fixing this Kevin. 🍰

@kevva kevva deleted the fix-surrogate-and-full-width branch July 22, 2017 22:40
@kevva
Copy link
Contributor Author

kevva commented Jul 22, 2017

Thanks 🎈

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.

Support for unicode surrogate pairs Support fullwidth characters
3 participants