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

getInitials: handle more cases to reduce cross-platform inconsistencies #5029

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Conversation

kkstrk
Copy link
Contributor

@kkstrk kkstrk commented Feb 15, 2021

First time contributor checklist:

Contributor checklist:

Description

#4172

I made changes to the getInitials method to handle more cases. The first letter of the first and the last word in a passed string will form the initials. Any non-letters, suffixes such as "Jr", "Sr" or Roman numerals will be omitted.
I added tests and tried to cover multiple edge cases.

OS X 10.15.6.

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I've made a couple of comments. When in doubt, making this match Android's codebase is ideal.

ts/util/getInitials.ts Outdated Show resolved Hide resolved
ts/test-both/util/getInitials_test.ts Show resolved Hide resolved
@EvanHahn-Signal
Copy link
Contributor

Thanks for making the updates. I might not get to it this week, but I'll take another look at your changes.

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

A few more comments. Thanks for doing this!

Comment on lines 47 to 52
[
['山田 太郎', '山太'],
['王五', '王五'],
['Иван Иванов', 'ИИ'],
].forEach(([name, initials]) => {
it('returns initials for languages with non-Latin alphabets', () => {
assert.strictEqual(getInitials(name), initials);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This forEach (and the one below) causes multiple it blocks with the same title. I think it might be clearer to do everything inline:

  it('returns initials for languages with non-Latin alphabets', () => {
    assert.strictEqual(getInitials('山田 太郎'), '山太');
    // ...
  });

Same comment applies to the RTL test below.

});

it('returns the first letter of a name that is one ASCII word', () => {
assert.strictEqual(getInitials('Foo'), 'F');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test for a two-letter name (like "Bo")? (I think that should be "B", not "BO".)


[
['山田 太郎', '山太'],
['王五', '王五'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? Are "王五" initials? Or should it be "王"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's correct. Those two signs are both uppercase letters.

@kkstrk
Copy link
Contributor Author

kkstrk commented Mar 9, 2021

@EvanHahn-Signal I'm sorry it's taken so long. I completely missed the notifications for these newer comments. I'll work on them this week.

@EvanHahn-Signal
Copy link
Contributor

No worries at all. Take your time!

@EvanHahn-Signal
Copy link
Contributor

I just saw that you pushed some more commits. Let me know if I should take another look.

@kkstrk
Copy link
Contributor Author

kkstrk commented Mar 27, 2021

Yes, please do. I made some changes to the test file based on your comments.

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

Looks good! We'll plan to merge this soon.

@EvanHahn-Signal EvanHahn-Signal merged commit d20cc59 into signalapp:development Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants