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

Added skin tone support for Emojis #4456

Merged
merged 62 commits into from
Sep 9, 2021

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Aug 6, 2021

@stitesExpensify Task complete. Can you review, please?

Details

Extended the existing Emoji Picker to add Skin Tone support.

Fixed Issues

$ #4323

Tests

  1. Tested several emojis with all skin tones
  2. Verified NVP is stored for Preferred Skin tone
  3. Verified NVP is available on refresh on the same device and other devices
  4. Tested Filter + Skintone
  5. Verified missing skin tones for all the emojis
  6. Tested default and null handling for emoji skin tones in case skin tone not available
  7. UI elements in terms of selection, on hover, refreshing the list tested
  8. UI matched with the mockup shared
  9. Tested with Landscape mode for Mobile Devices

QA Steps

  1. Go to chats and open EmojiPicker
  2. With the first attempt the default skin tone should show up.
  3. Choose the skin tone and verify that all the Emojis with Skin tone support have the chosen skin tone
  4. Select any emoji and it should render with the chosen skin tone
  5. Change the skin tone and the emojis should have the updated skin tone
  6. Login with another device and ensure that the chosen Emoji skin tone is reflected by default
  7. Try deleting the emoji and see that it doesn't require two deletes.
  8. Attempt to load the Emoji picker in the Mobile landscape view as well and see that emoji picker doesn't break
  9. Search the emojis with keywords. These should also show up the emojis in chosen skin tone only

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

emoji-skintone-web_nXawXt4H.mp4

Mobile Web

emoji-skintone-mweb_lkLVcSj5.mp4

emoji-skintone-mweb-landscape

Updated
Screenshot 2021-09-09 at 9 19 51 AM

Desktop

emoji-skintone-desktop.mp4

iOS

https://user-images.githubusercontent.com/3069065/128576156-2787a270-bdec-4fad-9db2-c4240b5c5211.mp4
emoji-skintone-ios-landscape

Updated
Screenshot 2021-09-09 at 9 26 41 AM

Android

emoji-skintone-android_loTYePmA.mp4

emoji-skintone-android-landscape

Updated
Screenshot 2021-09-09 at 9 32 11 AM

@mananjadhav mananjadhav requested a review from a team as a code owner August 6, 2021 14:43
@MelvinBot MelvinBot requested review from puneetlath and removed request for a team August 6, 2021 14:43
@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Sep 3, 2021

@stitesExpensify clarifying again:

  1. CONST.NVP. PREFERRED_EMOJI_SKIN_TONE: 'expensify_preferredEmojiSkinTone'
  2. ONXYKEYS.PREFERRED_EMOJI_SKIN_TONE: 'preferredSkinTone',

right?

@stitesExpensify
Copy link
Contributor

Let's just do

CONST.NVP. PREFERRED_EMOJI_SKIN_TONE: 'expensify_preferredEmojiSkinTone'
ONXYKEYS.PREFERRED_EMOJI_SKIN_TONE: 'preferredEmojiSkinTone',

@mananjadhav
Copy link
Collaborator Author

Ohh my, Sorry for the confusion but that's what I've done in the PR. I made a typo while putting it in a comment. Can you please review and close it?

@stitesExpensify
Copy link
Contributor

Everything is looking great! One more thing I found while testing is that the selector is not the same height as the button which makes the picker jump around

2021-09-08_12-13-30.mp4

@mananjadhav
Copy link
Collaborator Author

Okay noted. I'll check and update it.

@stitesExpensify
Copy link
Contributor

Thanks! I think after that we'll get this merged!

@mananjadhav
Copy link
Collaborator Author

@stitesExpensify PR updated.

Screen.Recording.2021-09-09.at.6.12.44.AM.mov

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Sep 9, 2021

@stitesExpensify I found another issue that the Skintone picker wasn't visible in the mobile landscape mode. This was because of another fix in the PR where we added some top margin so that the landscape mode picker works fine. I've had to change the approach to accommodate skin tone list. I guess now it is good to be merged with all the testing :)

Attached screenshots and updated in the PR body as well.

Screenshot 2021-09-09 at 9 19 51 AM

Screenshot 2021-09-09 at 9 26 41 AM

Screenshot 2021-09-09 at 9 32 11 AM

Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

LGTM!

@stitesExpensify
Copy link
Contributor

Can you write some QA steps? After that I'll merge :)

@mananjadhav
Copy link
Collaborator Author

@stitesExpensify Updated QA steps. Anything I've missed out?

@stitesExpensify
Copy link
Contributor

Nope those look great. Time to ship it! 😄

@stitesExpensify stitesExpensify merged commit dad9ddd into Expensify:main Sep 9, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Sep 9, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Sep 9, 2021

🚀 Deployed to staging by @stitesExpensify in version: 1.0.95-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@botify
Copy link

botify commented Sep 10, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-17. 🎊

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.0.96-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

}
style={[
styles.pv1,
styles.flex1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting this as flex: 1 caused #28917

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing out.

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.

8 participants