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

[HOLD for payment 2022-06-02] [$500] If you search a non existing emoji and press enter, then after deleting the search term it won't fetch emojis again - reported by @Tushu17 #8287

Closed
mvtglobally opened this issue Mar 24, 2022 · 60 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@mvtglobally
Copy link

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


Action Performed:

  1. Go to Chat > Emoji
  2. Search an non-existing emoji
  3. Press Enter
  4. Empty search box

Expected Result:

It should show emojis after clearing the search term.

Actual Result:

It shows 'No result found'

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Version Number: 1.1.44-4
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Sec-Rec01.1.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1647102086741009

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Mar 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Mar 24, 2022

Triggered auto assignment to @flaviadefaria (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 24, 2022
@sobitneupane
Copy link
Contributor

Proposal:
Addition of following line of code will solve the issue:

if (this.searchInput && !this.searchInput.isFocused()) {
this.setState({selectTextOnFocus: false});
this.searchInput.value = '';
this.searchInput.focus();
// Re-enable selection on the searchInput
this.setState({selectTextOnFocus: true});
}

            if (this.searchInput && !this.searchInput.isFocused()) {
                this.setState({selectTextOnFocus: false});
                this.searchInput.value = '';
                this.searchInput.focus();
+               this.filterEmojis(this.searchInput.value);
                
                // Re-enable selection on the searchInput
                this.setState({selectTextOnFocus: true});
            }

@melvin-bot
Copy link

melvin-bot bot commented Mar 25, 2022

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@flaviadefaria flaviadefaria removed their assignment Mar 25, 2022
@Luke9389 Luke9389 added the External Added to denote the issue can be worked on by a contributor label Mar 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Mar 25, 2022

Triggered auto assignment to @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@Luke9389 Luke9389 added Weekly KSv2 and removed Daily KSv2 labels Mar 25, 2022
@jliexpensify
Copy link
Contributor

jliexpensify commented Mar 28, 2022

Posted - I changed the title to fit in with Upwork's character limit, but kept the original title in this GH.

Internal - https://www.upwork.com/ab/applicants/1508261111247921152/job-details
External - https://www.upwork.com/jobs/~01b2bf9b46c621dad9

@melvin-bot
Copy link

melvin-bot bot commented Mar 28, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Mar 28, 2022

Current assignee @Luke9389 is eligible for the Exported assigner, not assigning anyone new.

@Puneet-here
Copy link
Contributor

Proposal

Need to remove this line

this.searchInput.value = '';

it will also solve one more problem. As of now when we press enter and type something the previous search term get's cleared instead it should stay and user should be able to modify it.

Before :-

Before.mp4

Problem: if the user mistyped and presses enter then the user will just correct the typo but the search term is getting cleared so the user has to type the whole search term again which is not a good ux.

After removing the line 154:-

After.mp4

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Mar 28, 2022

@Puneet-here
Sound goods. But on pressing enter focus should not be lost.
Can you update your proposal?

@mountiny mountiny changed the title If you search a non existing emoji and press enter, then after deleting the search term it won't fetch emojis again - reported by @Tushu17 [$250] If you search a non existing emoji and press enter, then after deleting the search term it won't fetch emojis again - reported by @Tushu17 Mar 31, 2022
@Puneet-here
Copy link
Contributor

Puneet-here commented Apr 3, 2022

@Puneet-here Sound goods. But on pressing enter focus should not be lost. Can you update your proposal?

we can pass onBlur={() => this.searchInput.focus()} in composer

<Composer
textAlignVertical="top"
placeholder={this.props.translate('common.search')}
placeholderTextColor={themeColors.textSupporting}
onChangeText={this.filterEmojis}

@Santhosh-Sellavel
Copy link
Collaborator

Will review this ASAP (today or by tomorrow).

@melvin-bot melvin-bot bot removed the Overdue label Apr 7, 2022
@eVoloshchak
Copy link
Contributor

@Luke9389

20220518_092019.mp4
  1. Search for a non-existing emoji
  2. Press Enter
  3. Press Backspace

@Luke9389
Copy link
Contributor

Got it, OK. Thanks

@Luke9389
Copy link
Contributor

I think I actually like @Puneet-here's idea to just take that line out.
It's strange that a single backspace press would clear the whole line.

@NehaSantani
Copy link

Hey what is the status of this issue?
Is it still open?
I can work on it!

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented May 20, 2022

@Luke9389,

Especially when searching emojis we're gonna type 2- 5 characters not more than 10 - 15 characters. Moreover, we are not clear on everything on each backspace, only after the user press enter and backspace. Considering we are not searching for anything important like a contact etc. Clearing doesn’t sound strange at all to me.

@Puneet-here
Copy link
Contributor

I don't think we should clear the search term even if the search term isn't 10-15 character long. Why we wanna clear it when we should be keeping the search term there as the inputs are expected to work like that?

I don't think these are fair points for why we should clear the search term. We can solve this issue and improve something then why should we go with a workaround to solve only the issue?

@Santhosh-Sellavel
Copy link
Collaborator

@Luke9389

I had started a slack 🧵 regarding the confusion here about clearing or keeping the invalid search input and modifying it.
Most of the team prefer modifying the text consensus reach in the poll here

So Let's go with @Puneet-here proposal here!
🎀 👀 🎀 C+ reviewed

Thanks!

@Luke9389
Copy link
Contributor

Ok, go ahead and start your PR @Puneet-here 👍

@Puneet-here
Copy link
Contributor

I have raised the PR. Thanks.

@Puneet-here
Copy link
Contributor

Hi @JmillsExpensify, the previous job has expired can you please open a new one.

@Santhosh-Sellavel
Copy link
Collaborator

@Puneet-here I am not seeing any PR mentioned this issue
Can you share the PR Link and have you linked the issue properly?

@Puneet-here
Copy link
Contributor

Puneet-here commented May 24, 2022

Hmm strange, didn't notice that the PR didn't get mentioned here. Here's the PR

@Luke9389
Copy link
Contributor

Yeah, that's weird. 🤔

@Puneet-here
Copy link
Contributor

I think this is happening with all the recent PRs

@JmillsExpensify
Copy link

Hi @JmillsExpensify, the previous job has expired can you please open a new one.

Here you go! https://www.upwork.com/jobs/~019a6d1271f2483032 cc @Santhosh-Sellavel

@Puneet-here
Copy link
Contributor

Here you go! https://www.upwork.com/jobs/~019a6d1271f2483032 cc @Santhosh-Sellavel

Thank you @JmillsExpensify, I have applied to the job.

@JmillsExpensify
Copy link

@Puneet-here @Santhosh-Sellavel offers sent. @Tushu17 Can you apply as well for reporting? Thank you!

@Tushu17
Copy link
Contributor

Tushu17 commented May 25, 2022

@JmillsExpensify Applied, Thanks.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 26, 2022
@melvin-bot melvin-bot bot changed the title [$500] If you search a non existing emoji and press enter, then after deleting the search term it won't fetch emojis again - reported by @Tushu17 [HOLD for payment 2022-06-02] [$500] If you search a non existing emoji and press enter, then after deleting the search term it won't fetch emojis again - reported by @Tushu17 May 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.67-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-06-02. 🎊

@JmillsExpensify
Copy link

Almost through the regression hold. Will circle back here in a couple of days for payment.

@Santhosh-Sellavel
Copy link
Collaborator

Lol, We didn’t assign this to @Puneet-here.
@JmillsExpensify
Please assign it to them before closing this out.
So far no regressions. Payment is due tomorrow.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 1, 2022
@JmillsExpensify
Copy link

Thanks! Payments issued via Upwork to all contributors. Have a great weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests