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

Improve world-readiness of initials algorithm #1106

Merged
merged 17 commits into from Feb 25, 2017

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Contributor

commented Feb 24, 2017

Pull request checklist

  • Addresses an existing issue: #0000
  • Include a change request file if publishing
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Description of changes

This change improves the world-readiness of the initials algorithm.

Specifically, it adds some special-case handling for:

  1. Chinese and Korean names for which the rules for latin-alphabet names
    produce non-sensical results.

  2. Arabic names for which the latin-rules produce potentially offensive
    results due to ligature contraction.

Note that both of these world-readiness fixes are guaranteed to keep the existing contract that the initials function returns at most 2 characters.

Focus areas to test

Persona component, especially with non-Latin scripts.

c-w and others added some commits Feb 22, 2017

Move initials creation logic to utility
This will enable easy unit testing, re-use, etc. going forward.
Improve world-readiness of initials algorithm
Add some special-case handling for:

1) Chinese and Korean names for which the rules for latin-alphabet names
produce non-sensical results.

2) Arabic names for which the latin-rules produce potentially offensive
results due to ligature contraction.

Note that both of these world-readiness fixes are guaranteed to keep the
existing contract that the initials function returns at most 2 characters.
@msftclas

This comment has been minimized.

Copy link

commented Feb 24, 2017

@c-w,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2017

Putting this onto the radar of @cliffkoh.

@cliffkoh

This comment has been minimized.

Copy link
Member

commented Feb 24, 2017

@c-w Thanks for the PR. Just wondering, did you get a chance to test if 2 asian characters (Chinese or Korean) would fit within the Persona textboy? We could possibly augment the Persona example page to include such an example which would also provide a convenient way to validate this scenario...

});

it('calculates an expected initials for Chinese names', () => {
let result = getInitials('桂英', false);

This comment has been minimized.

Copy link
@cliffkoh

cliffkoh Feb 24, 2017

Member

Very common for Chinese to have 3 characters, sometimes even 4. It would be great to have one such test, just to ensure for instance that no more than 2 characters are returned.

This comment has been minimized.

Copy link
@c-w

c-w Feb 24, 2017

Author Contributor

Good catch. Will add another test.

This comment has been minimized.

Copy link
@c-w

c-w Feb 24, 2017

Author Contributor

Added test in f414fd5.

@cliffkoh
Copy link
Member

left a comment

Thank you for this, could you please include a change file as well? (which can be generated using rush change, or you could copy the change file format from another PR if not using the rush tool)

c-w added some commits Feb 24, 2017

Add test for long Chinese names
We need to ensure that even for names with three or more characters, the
generated initials don't contain more than two characters.
@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2017

Here's a render of the persona component with non-Latin names:

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2017

When I run ./node_modules/.bin/rush change, I get told that "No change file is needed.", so I'll create the file manually.

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2017

@cliffkoh: Addressed all the comments. Could you please take another look?

{
"packageName": "office-ui-fabric-react",
"comment": "Improve abbreviation of non-Latin names in Persona component",
"type": "minor"

This comment has been minimized.

Copy link
@cliffkoh

cliffkoh Feb 25, 2017

Member

strictly speaking, a patch change because the API surface did not change

@cliffkoh cliffkoh merged commit a58e88d into OfficeDev:master Feb 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.