Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$500] Emojis - "No results found" message appears when select "Hand" from keyboard suggestion #31780

Closed izarutskaya closed 11 months ago

izarutskaya commented 11 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.2-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation: @

Action Performed:

  1. Open New Expensify app
  2. Open any conversation
  3. Navigate to emoji picker
  4. Type "hand" into search bar
  5. Choose emoji from keyboard suggestion

Expected Result:

Selected emoji should be applied and not error messages should appear

Actual Result:

"No results found" message appears when select "Hand" from keyboard suggestion

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/5857c889-495f-489b-8814-b49deffe0dcc

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01018ef056ade09956
  • Upwork Job ID: 1727652489576235008
  • Last Price Increase: 2023-12-07
melvin-bot[bot] commented 11 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01018ef056ade09956

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @burczu (External)

paultsimura commented 11 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When searching by an actual emoji, the EmojiPicker doesn't suggest an emoji with an exact match.

What is the root cause of that problem?

In the EmojiPickerMenu, we do not support search by an actual emoji, only by matching the normalized text.

What changes do you think we should make in order to solve the problem?

We should check if the searchTerm is an emoji. If yes – look for an emoji with the exact match of the emoji.code using EmojiUtils.findEmojiByCode instead of an emoji suggestion by text.

We can achieve this by making the following changes (or splitting the logic into separate assignments, but the general idea is the same):

const newFilteredEmojiList = EmojiUtils.containsOnlyEmojis(normalizedSearchTerm) 
            ? [EmojiUtils.findEmojiByCode(normalizedSearchTerm)].filter(Boolean) 
            : EmojiUtils.suggestEmojis(`:${normalizedSearchTerm}`, preferredLocale, emojis.current.length);

https://github.com/Expensify/App/blob/03a9fe09d8d736b5fc268e885a5f56b878846074/src/components/EmojiPicker/EmojiPickerMenu/index.js#L181

Result:

https://github.com/Expensify/App/assets/12595293/75ccd545-474d-4684-b7f3-f087187bd5da

What alternative solutions did you explore? (Optional)

If we want to support a non-strict search, but to search for multiple emojis at the same time, we can instead use the following:

   const newFilteredEmojiList = EmojiUtils.containsOnlyEmojis(normalizedSearchTerm)
            ? EmojiUtils.extractEmojis(normalizedSearchTerm)
            : EmojiUtils.suggestEmojis(`:${normalizedSearchTerm}`, preferredLocale, emojis.current.length);
image
ta106 commented 11 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we search using an emoji we don't realize that it is the one that should be selected.

What is the root cause of that problem?

In the EmojiPickerMenu.native.js

const filterEmojis = _.debounce((searchTerm) => {
        const normalizedSearchTerm = searchTerm.toLowerCase().trim().replaceAll(':', '');

        if (emojiList.current) {
            emojiList.current.scrollToOffset({offset: 0, animated: false});
        }

        if (normalizedSearchTerm === '') {
            setFilteredEmojis(allEmojis);
            setHeaderIndices(headerRowIndices);

            return;
        }
        const newFilteredEmojiList = EmojiUtils.suggestEmojis(`:${normalizedSearchTerm}`, preferredLocale, allEmojis.length);
        setFilteredEmojis(newFilteredEmojiList);
        setHeaderIndices(undefined);
    }, 300);

we don't catch if an emoji is selected

What changes do you think we should make in order to solve the problem?

We should check if the searchTerm is an emoji, we should make it the selected emoji to be pushed We can do this by calling onEmojiSelected if the search is an emoji

const emojiCode = EmojiUtils.findEmojiByCode(normalizedSearchTerm);
if (emojiCode) {
     onEmojiSelected(emojiCode.code, emojiCode);
     return;
}

What alternative solutions did you explore? (Optional)

Maybe we do something like discord where we disable the native emoji suggestions

yh-0218 commented 11 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we search using an emoji we don't realize that it is the one that should be selected.

What is the root cause of that problem?

Because we didn't handle emoji search when input emoji. https://github.com/Expensify/App/blob/0cca90ab9aa7fd9467ef45ca35c4d0ad3c3334ae/src/components/EmojiPicker/EmojiPickerMenu/index.native.js#L84 and https://github.com/Expensify/App/blob/0cca90ab9aa7fd9467ef45ca35c4d0ad3c3334ae/src/components/EmojiPicker/EmojiPickerMenu/index.js#L181

What changes do you think we should make in order to solve the problem?

We need to handle like this.

const convertedSearchTerm  = EmojiUtils.findEmojiByCode(normalizedSearchTerm)
        ? EmojiUtils.getEmojiName(EmojiUtils.findEmojiByCode(normalizedSearchTerm))
        :normalizedSearchTerm

const newFilteredEmojiList = EmojiUtils.suggestEmojis(`:${convertedSearchTerm}`, preferredLocale, emojis.current.length);

What alternative solutions did you explore? (Optional)

https://github.com/Expensify/App/assets/65789015/813be777-08ed-4b5e-b4e1-59742e8643a2

yh-0218 commented 11 months ago

This is a Emojis suggesting Modal.

twisterdotcom commented 11 months ago

Man this is super niche. @burczu any of these proposals good? Going to downgrade to Weekly though.

DylanDylann commented 11 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

const filterEmojis = _.throttle((searchTerm) => {

...

}, throttleTime);
- Also, we need to handle the case users typing more than one emoji. The general idea is when user type "✋😀abc", it will create an array  ['✋', '😀', 'a', 'b', 'c'] and check if the element is emoji or not. If true, replace it with an emoji name (Ex: ✋ => ```:hand:```). Finally, join the result array  ```[':hand:', ':grinning:', 'a', 'b', 'c']``` to a string. For example, the above code change can become:
````diff
+    const [searchTermValue, setSearchTermValue] = useState('');
const filterEmojis = _.throttle((searchTerm) => {
+  let originalSearchTerm = searchTerm;
+        let originalSearchTermCharacters = [...originalSearchTerm];
+        originalSearchTermCharacters.forEach((character, index) => {
+            if (EmojiUtils.containsOnlyEmojis(character)) {
+                const emojiName = EmojiUtils.findEmojiByCode(character)?.name;
+                if (emojiName) {
+                    originalSearchTermCharacters[index] = `:${emojiName}:`;
+                }
+            }
+        });
+        originalSearchTerm = originalSearchTermCharacters.join('');
+        const normalizedSearchTerm = originalSearchTerm.toLowerCase().trim().replaceAll(':', '');
+        setSearchTermValue(originalSearchTerm);

What alternative solutions did you explore? (Optional)

Result

Screencast from 24-11-2023 00:55:29.webm

twisterdotcom commented 11 months ago

Not overdue, also not daily.

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

burczu commented 11 months ago

@twisterdotcom I like the most the proposal from @paultsimura but I agree it's very niche and I'm not sure if we should fix this. I've checked on Slack and it works the same as in Expensify App - please see the video below:

https://github.com/Expensify/App/assets/7771498/2056ab00-5f87-42df-92cc-3975ecb20964

DylanDylann commented 11 months ago

@burczu

Screencast from 04-12-2023 18:20:37.webm

burczu commented 11 months ago

@DylanDylann So we should not change how we search for emojis, but how we copy/paste them, right? Right now, if you copy emoji and paste it to the search input, it shows up as emoji, but it should appear as the code - it should work as expected then.

DylanDylann commented 11 months ago

@burczu

So we should not change how we search for emojis, but how we copy/paste them, right

Right now, if you copy emoji and paste it to the search input, it shows up as emoji, but it should appear as the code - it should work as expected then.

  • There are two cases we need to consider:
    1. Copy the emoji from another app, in slack, 👍 is still 👍 (same as Expensive)
    2. Copy the emoji from native app, in slack, 👍 will display as :+1: (in Expensive, 👍 is still 👍)
burczu commented 11 months ago

Yes, but I still don't think it's worth fixing - on native devices users will use emoji pickers embedded in the screen keyboard, not our custom emoji picker; on desktop they will use keyboard to search for emoji because if they can copy emoji from somewhere else, why would they search it in the emoji picker if they can just paste it in the editor input?

DylanDylann commented 11 months ago

@burczu Yeah. It's just my opinion.

if they can copy emoji from somewhere else, why would they search it in the emoji picker if they can just paste it in the editor input?

Screencast from 04-12-2023 18:46:03.webm

twisterdotcom commented 11 months ago

Look, I think the benefit of having a great open source codebase is that we can fix anything, no matter how small. There's no time restriction on this one, but if we have a fix that can make our cross platform app work consistently, I think it's fine for us to "fix" it.

paultsimura commented 11 months ago

If this is worth fixing, I don't see any benefit in complicating things with the way we copy-paste the emojis in the emoji search bar instead of using the direct emoji matching. From my user experience, it did happen sometimes that I was trying to look for an emoji in the emoji picker, entered the (half-)name, and tapped the autocompletion – when the emoji is inserted into the picker search bar, I would expect an easy way to "transfer" it from the emoji picker into the composer – and in this case, the 1:1 match is the way to go IMHO.

situchan commented 11 months ago

I remember this was closed as not bug. Finding GH

situchan commented 11 months ago

Dupe of https://github.com/Expensify/App/issues/24399

paultsimura commented 11 months ago

Dupe of #24399

With all due respect, this issue is different as it has nothing to do with the copy-paste behavior of the emojis within the app – it's only related to the EmojiPicker's search logic.

melvin-bot[bot] commented 11 months ago

@burczu @twisterdotcom this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 11 months ago

@burczu, @twisterdotcom Eep! 4 days overdue now. Issues have feelings too...

burczu commented 11 months ago

@DylanDylann I like what we discussed above - is it reflected in your proposal already?

DylanDylann commented 11 months ago

@burczu

I like what we discussed above

Which discussion are you mentioning?

burczu commented 11 months ago

@DylanDylann Like, here and here

twisterdotcom commented 11 months ago

Hmm https://github.com/Expensify/App/issues/24399 does have some interesting discussion about the complexity of the fix vs the likelihood for real users encountering this.

Things are moving fast here at Expensify HQ and we are moving towards a world where only real world reported bugs will be fixed, so I'm here to retract my previous viewpoint and say that I think given this is so unlikely to affect a real world user, we should close and perhaps when we come back to fixing every little thing, we'll do this then.

@burczu and @DylanDylann and everybody else, I definitely appreciate the discussion on this.