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

fix: sorted suggestion emoji #51220

Merged
merged 11 commits into from
Nov 19, 2024
Merged

fix: sorted suggestion emoji #51220

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e774111
fix: sorted suggestion emoji
nkdengineer Oct 22, 2024
a2adfcd
Merge branch 'main' into fix/50079
nkdengineer Oct 22, 2024
596f16f
fix test
nkdengineer Oct 22, 2024
313d716
chore
nkdengineer Oct 22, 2024
8e6b360
fix test
nkdengineer Oct 22, 2024
7cacc00
fix lint
nkdengineer Oct 22, 2024
542cb41
Merge branch 'main' into fix/50079
nkdengineer Oct 30, 2024
c0a7a2f
create a function to remove dup code
nkdengineer Oct 30, 2024
14e4caf
Merge branch 'main' into fix/50079
nkdengineer Nov 6, 2024
ebcdade
Merge branch 'main' into fix/50079
nkdengineer Nov 11, 2024
3120da7
Merge branch 'main' into fix/50079
nkdengineer Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/libs/EmojiUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {Str} from 'expensify-common';
import lodashSortBy from 'lodash/sortBy';
import Onyx from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import * as Emojis from '@assets/emojis';
Expand All @@ -23,6 +24,8 @@ const findEmojiByName = (name: string): Emoji => Emojis.emojiNameTable[name];

const findEmojiByCode = (code: string): Emoji => Emojis.emojiCodeTableWithSkinTones[code];

const sortByName = (emoji: Emoji, emojiData: RegExpMatchArray) => !emoji.name.includes(emojiData[0].toLowerCase().slice(1));

let frequentlyUsedEmojis: FrequentlyUsedEmoji[] = [];
Onyx.connect({
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
Expand Down Expand Up @@ -424,7 +427,7 @@ function suggestEmojis(text: string, lang: Locale, limit: number = CONST.AUTO_CO
for (const node of nodes) {
if (node.metaData?.code && !matching.find((obj) => obj.name === node.name)) {
if (matching.length === limit) {
return matching;
return lodashSortBy(matching, (emoji) => sortByName(emoji, emojiData));
}
matching.push({code: node.metaData.code, name: node.name, types: node.metaData.types});
}
Expand All @@ -434,15 +437,15 @@ function suggestEmojis(text: string, lang: Locale, limit: number = CONST.AUTO_CO
}
for (const suggestion of suggestions) {
if (matching.length === limit) {
return matching;
return lodashSortBy(matching, (emoji) => sortByName(emoji, emojiData));
}

if (!matching.find((obj) => obj.name === suggestion.name)) {
matching.push({...suggestion});
}
}
}
return matching;
return lodashSortBy(matching, (emoji) => sortByName(emoji, emojiData));
}

/**
Expand Down
10 changes: 5 additions & 5 deletions tests/unit/EmojiTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,11 @@ describe('EmojiTest', () => {

it('correct suggests emojis accounting for keywords', () => {
const thumbEmojisEn: Emoji[] = [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we move this back up? We moved it down for this issue: #48748

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srikarparsi After we sort the suggestion emoji, the position of the emojis in the test is changed here

Copy link
Contributor

@srikarparsi srikarparsi Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, when I test it manually, the wrong emoji comes up first, does this occur for you as well?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srikarparsi I think that's expected since we're prioritizing sorting by emoji name here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't think that's expected. Based on this issue, I believe we want :thumb to show the thumbs up emoji first instead of the hand_with_index_finger emoji in the picture.

Copy link
Contributor

Choose a reason for hiding this comment

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

My vote would be either to do nothing, or update the sorting algorithm now to include keywords since "thumbsup" is already in that array.
2024-11-19_14-02-07

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, :thumbsup: is the emoji name on slack

The name in slack is :+1: 😄 Even if you type :thumbsup:, when you highlight it, it will always say +1

2024-11-19_14-03-05

Copy link
Contributor

@srikarparsi srikarparsi Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm okay, updating the sorting algorithm to include keywords wouldn't work for this example right? Because melting_face 🫠 has keywords that start with dis even though the name doesn't.
image

I am leaning towards changing the names of +1 and -1 to thumbsup and thumbsdown and including a comment explaining to change the name back once we incorporate frequency into our sorting algorithm. Does that sound okay with you?

If not, I am also fine with doing nothing for now for this edge case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah let's do nothing. Keeping the same names as unicode seems more important than this one edge case IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Soundss good, going to merge this one then

name: 'hand_with_index_finger_and_thumb_crossed',
code: '🫰',
types: ['🫰🏿', '🫰🏾', '🫰🏽', '🫰🏼', '🫰🏻'],
},
{
code: '👍',
name: '+1',
Expand All @@ -164,11 +169,6 @@ describe('EmojiTest', () => {
name: '-1',
types: ['👎🏿', '👎🏾', '👎🏽', '👎🏼', '👎🏻'],
},
{
name: 'hand_with_index_finger_and_thumb_crossed',
code: '🫰',
types: ['🫰🏿', '🫰🏾', '🫰🏽', '🫰🏼', '🫰🏻'],
},
];

const thumbEmojisEs: Emoji[] = [
Expand Down
Loading