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

Added frequently used emojis support #6498

Merged
merged 13 commits into from
Dec 7, 2021
Merged

Added frequently used emojis support #6498

merged 13 commits into from
Dec 7, 2021

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Nov 26, 2021 •
edited
Loading

Details

  • Added support for Frequently Used Emojis and Recently used emojis
  • Belongs to a common list and it is synced across sessions with NameValuePairs

Fixed Issues

$ #4559

Tests

  1. Tested adding multiple emojis to recent
  2. Tested emojis by multiple counts (frequency)
  3. Tested by limiting the emoji count and relevant replacement logic in case of more emojis than the given limit
  4. Tested sync across sessions for Frequent Emoji List
  5. Tested preferred skin tone along with frequent emojis

QA Steps

  1. Go to any chat and open EmojPicker
  2. Select your favorite emoji and add it to Text
  3. Open the EmojiPicker again and see it as a part of the Frequently Used Emojis
  4. Add multiple emojis to see that the order is either based on the frequency or recently added emojis

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-frequent-emojis

Mobile Web

mweb-frequent-emojis

Desktop

desktop-frequent-emojis

iOS

ios-frequent-emojis

Android

android-frequent-emojis

@mananjadhav
Copy link
Collaborator Author

@stitesExpensify While my work is done, I've got a regression from the existing main branch, where EmojiPicker has some extra padding on the left.

image

@mananjadhav mananjadhav marked this pull request as ready for review November 26, 2021 20:59
@mananjadhav mananjadhav requested a review from a team as a code owner November 26, 2021 20:59
@MelvinBot MelvinBot removed the request for review from a team November 26, 2021 20:59
@mananjadhav
Copy link
Collaborator Author

@stitesExpensify I've fixed the above padding issue with a small fix in Popover/index.native.js. Please review that as well.

@mananjadhav mananjadhav mentioned this pull request Nov 30, 2021
Closed
@Jag96
Copy link
Contributor

Jag96 commented Nov 30, 2021

@mananjadhav #6523 has fixed the padding issue so if this commit is no longer necessary please remove it

@mananjadhav
Copy link
Collaborator Author

@Jag96 yeah it won't be needed I'll take the latest pull of main and remove my commit.

@mananjadhav
Copy link
Collaborator Author

Commit removed and PR updated. @stitesExpensify Any updates for me on this one?

@Jag96
Copy link
Contributor

Jag96 commented Dec 1, 2021

@stitesExpensify is OOO, assigning pullerbear to get another reviewer on this one

@Jag96 Jag96 requested a review from a team December 1, 2021 22:44
@MelvinBot MelvinBot removed the request for review from a team December 1, 2021 22:44
@Jag96 Jag96 self-requested a review December 1, 2021 22:51
Jag96
Jag96 reviewed Dec 2, 2021
View reviewed changes
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Works well! A few requests, will let @stitesExpensify do a final review when he's back from OOO

src/libs/EmojiUtils.js Outdated
@@ -67,9 +70,97 @@ function isSingleEmoji(message) {
return matchedUnicode === currentMessageUnicode;
}

/**
* Get the header indices based on the max emojis per row
* @param {Array} emojis
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you specify the array type here? For example {Object[]} or {String[]}. Same for other usages of {Array}

src/ONYXKEYS.js Outdated
@@ -133,6 +133,9 @@ export default {
// Store preferred skintone for emoji
PREFERRED_EMOJI_SKIN_TONE: 'preferredEmojiSkinTone',

// Store frequently used emojis user wise
Copy link
Contributor

Choose a reason for hiding this comment

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

nab - Store frequently used emojis for this user

src/libs/EmojiUtils.js Outdated
import CONST from '../CONST';
import * as UserActions from './actions/User';
Copy link
Contributor

Choose a reason for hiding this comment

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

Update UserActions to User to keep consistency with other usages

src/libs/EmojiUtils.js Outdated
}

/**
* Get a merged array if frequently used emojis exist
Copy link
Contributor

Choose a reason for hiding this comment

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

of instead of if

@mananjadhav
Copy link
Collaborator Author

@Jag96 PR updated

Jag96
Jag96 suggested changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Just one missed usage, otherwise LGTM

src/libs/actions/User.js Outdated
@@ -317,6 +321,15 @@ function setPreferredSkinTone(skinTone) {
return NameValuePair.set(CONST.NVP.PREFERRED_EMOJI_SKIN_TONE, skinTone, ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE);
}

/**
* Sync frequentlyUsedEmojis with Onyx and Server
* @param {Array} frequentlyUsedEmojis
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed an {Array} usage here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry about that. Fixed and updated PR.

Jag96
Jag96 approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Thanks @mananjadhav, this LGTM, @stitesExpensify when you get back from OOO feel free to leave a review if there are any follow-ups you'd like addressed.

@Jag96 Jag96 merged commit 04d28af into Expensify:main Dec 7, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 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.

@stitesExpensify
Copy link
Contributor

🔥 I didn't review the code but I just tested it and it seems to work great! Thanks for the work @mananjadhav and thanks for taking over the review for me @Jag96 I appreciate it

@OSBotify OSBotify mentioned this pull request Dec 8, 2021
Closed
25 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to staging by @Jag96 in version: 1.1.18-4 🚀

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

@botify botify mentioned this pull request Dec 20, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

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

@sobitneupane sobitneupane mentioned this pull request Jul 24, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jag96 Jag96 Jag96 approved these changes

@stitesExpensify stitesExpensify Awaiting requested review from stitesExpensify

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

Successfully merging this pull request may close these issues.
4 participants
@mananjadhav @Jag96 @OSBotify @stitesExpensify