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.

Sign up for GitHub

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

Jump to bottom

fix(Avatar): support display names using emoji and multi-byte #1716

Merged
merged 1 commit into from
Jul 31, 2023
Merged

fix(Avatar): support display names using emoji and multi-byte #1716

merged 1 commit into from
Jul 31, 2023

Conversation

booc0mtaco
Copy link
Contributor

Summary:

  • add new prop for display name to allow for avatar text not based on the user's real name
  • add tests for sensible fallbacks when using this component

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:

@booc0mtaco booc0mtaco requested a review from a team July 31, 2023 21:49
@codecov
Copy link

codecov bot commented Jul 31, 2023 •
edited
Loading

Codecov Report

Merging #1716 (36f8806) into next (8e4e462) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             next    #1716      +/-   ##
==========================================
+ Coverage   92.33%   92.47%   +0.13%     
==========================================
  Files         143      143              
  Lines        2493     2498       +5     
  Branches      637      640       +3     
==========================================
+ Hits         2302     2310       +8     
+ Misses        175      172       -3     
  Partials       16       16
Files Changed Coverage Δ
src/components/Avatar/Avatar.tsx 100.00% <100.00%> (+12.50%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Jul 31, 2023 •
edited
Loading

size-limit report 📦

Path Size
components 95.49 KB (+0.11% 🔺)
styles 32.76 KB (+0.03% 🔺)

jinlee93
jinlee93 approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@jinlee93 jinlee93 left a comment

Choose a reason for hiding this comment

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

Think this makes sense to me, thanks!

booc0mtaco
booc0mtaco commented Jul 31, 2023
View reviewed changes
src/components/Avatar/Avatar.tsx
@@ -56,7 +60,7 @@ function getInitials(fromName: string): string {
* - User's name has a middle name or initial: John C. Smith
* - User's Name has dashes in it
*/
return fromName
const initials = fromName
.split(' ')
.map((part) => part[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of how the emoji get messed up, THIS line is the culprit. It will split an emoji into separate parts, leaving some unable to be rendered. Pragmatically, not touching this for now, as the use case for parsing a name with emoji IN the name is pretty niche. We should allow a display name in case people want to use non-name text for the user

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a regex is better for finding initials

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I found a regexp that could identify emoji as well that i didn't like AT ALL, so avoided going down that path. It included a bunch of specific escape characters that catch many (not all, maybe most) emoji.

But doing some additional digging yield this new thing i did not know about!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Unicode_character_class_escape

I'll test this out separately.

@booc0mtaco booc0mtaco force-pushed the aholloway/EDS-1081 branch 2 times, most recently from ef8c269 to 279452b Compare July 31, 2023 22:30
@booc0mtaco booc0mtaco force-pushed the aholloway/EDS-1081 branch from 279452b to 36f8806 Compare July 31, 2023 22:31
@booc0mtaco booc0mtaco merged commit 1294022 into next Jul 31, 2023
@booc0mtaco booc0mtaco deleted the aholloway/EDS-1081 branch July 31, 2023 23:43
@booc0mtaco booc0mtaco mentioned this pull request Aug 14, 2023
Merged
booc0mtaco added a commit that referenced this pull request Aug 15, 2023
@booc0mtaco
Merge pull request #1723 from chanzuckerberg/release-v13.0.0
c935b4f 
## [13.0.0](v12.4.2...v13.0.0) (2023-08-14)


### ⚠ BREAKING CHANGES

* **link:** remove text-link tokens for link t3 tokens (#1639)
* **colors:** remove old colors and convert to input and show figma token (#1711)
* remove deprecated dropdown (#1657)
* **banner:** remove component (#1702)
* remove legacy tokens and typography mixins (#1709)
* **Grid:** remove top-level sub-component(s) (#1703)
* **dragdrop:** remove top level subcomponents (#1697)
* **Fieldset:** remove top-level sub-component(s) (#1695)
* **HorizontalStepper:** remove top-level sub-component(s) (#1696)
* **Card:** remove top-level sub-component(s) (#1692)
* **checkbox:** remove top level subcomponents (#1693)
* **radio:** remove top level subcomponents (#1690)
* **DataBar:** remove top-level sub-component(s) (#1686)
* **Modal:** remove top-level sub-component(s) (#1689)
* **searchbar:** remove top level subcomponents (#1687)
* **table:** remove top-level sub-components (#1685)
* **toolbar:** remove component (#1683)
* **Breadcrumbs:** remove top-level sub-component (#1680)
* **timelinenav:** remove top-level sub-component (#1681)

### Features

* **Breadcrumbs:** remove top-level sub-component ([#1680](#1680)) ([669081d](669081d))
* **Card:** remove top-level sub-component(s) ([#1692](#1692)) ([7ec01f4](7ec01f4))
* **checkbox:** remove top level subcomponents ([#1693](#1693)) ([87b12e8](87b12e8))
* **DataBar:** remove top-level sub-component(s) ([#1686](#1686)) ([b4b9276](b4b9276))
* **dragdrop:** remove top level subcomponents ([#1697](#1697)) ([b4fd00c](b4fd00c))
* **Fieldset:** remove top-level sub-component(s) ([#1695](#1695)) ([0c8280d](0c8280d))
* **Grid:** remove top-level sub-component(s) ([#1703](#1703)) ([c8925c9](c8925c9))
* **HorizontalStepper:** remove top-level sub-component(s) ([#1696](#1696)) ([188fd99](188fd99))
* **Layout:** mark layout components as deprecated ([#1700](#1700)) ([930a369](930a369))
* **Modal:** remove top-level sub-component(s) ([#1689](#1689)) ([8743e62](8743e62))
* **radio:** remove top level subcomponents ([#1690](#1690)) ([82da617](82da617))
* remove legacy tokens and typography mixins ([#1709](#1709)) ([ec3e819](ec3e819))
* **searchbar:** remove top level subcomponents ([#1687](#1687)) ([d13bb6c](d13bb6c))
* **table:** remove top-level sub-components ([#1685](#1685)) ([742a530](742a530))
* **timelinenav:** remove top-level sub-component ([#1681](#1681)) ([f46eca7](f46eca7))


### Bug Fixes

* **Avatar:** support display names using emoji and multi-byte ([#1716](#1716)) ([1294022](1294022))
* update token exports ([#1722](#1722)) ([982c55f](982c55f))


* **banner:** remove component ([#1702](#1702)) ([356550c](356550c))
* **colors:** remove old colors and convert to input and show figma token ([#1711](#1711)) ([c9a5079](c9a5079))
* **link:** remove text-link tokens for link t3 tokens ([#1639](#1639)) ([d35cfe4](d35cfe4))
* remove deprecated dropdown ([#1657](#1657)) ([26d1694](26d1694))
* **toolbar:** remove component ([#1683](#1683)) ([bd47899](bd47899))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinlee93 jinlee93 jinlee93 approved these changes

@ahuth ahuth ahuth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@booc0mtaco @ahuth @jinlee93