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
Merged

Improve world-readiness of initials algorithm #1106

merged 17 commits into from Feb 25, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w 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 12 commits Feb 22, 2017
This will enable easy unit testing, re-use, etc. going forward.
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
Copy link

@msftclas msftclas 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
Copy link
Contributor Author

@c-w c-w commented Feb 24, 2017

Putting this onto the radar of @cliffkoh.

@cliffkoh
Copy link
Contributor

@cliffkoh cliffkoh 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);
Copy link
Contributor

@cliffkoh cliffkoh Feb 24, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@c-w c-w Feb 24, 2017

Choose a reason for hiding this comment

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

Good catch. Will add another test.

Copy link
Contributor Author

@c-w c-w Feb 24, 2017

Choose a reason for hiding this comment

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

Added test in f414fd5.

Copy link
Contributor

@cliffkoh cliffkoh 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 2 commits Feb 24, 2017
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
Copy link
Contributor Author

@c-w c-w commented Feb 24, 2017

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

@c-w
Copy link
Contributor Author

@c-w c-w 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
Copy link
Contributor Author

@c-w c-w 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"
Copy link
Contributor

@cliffkoh cliffkoh Feb 25, 2017

Choose a reason for hiding this comment

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

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

@cliffkoh cliffkoh merged commit a58e88d into microsoft:master Feb 25, 2017
1 check passed
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants