-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 2023-10-30] [$500] Search text is not selected in emoji search on refocus #28379
Comments
Triggered auto assignment to @sakluger ( |
Job added to Upwork: https://www.upwork.com/jobs/~0172e2de79a71b9db8 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Search text is not selected in emoji search on refocus What is the root cause of that problem?We already have prop
we only set to true in two places, but not in amount. App/src/components/EmojiPicker/EmojiPickerMenu/index.js Lines 205 to 212 in d7ad305
App/src/components/EmojiPicker/EmojiPickerMenu/index.js Lines 269 to 276 in d7ad305
What changes do you think we should make in order to solve the problem?We need to set default value for this.state = {
...
selectTextOnFocus: true,
} What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Search text is not selected in emoji search on refocus What is the root cause of that problem?We are missing state value of What changes do you think we should make in order to solve the problem?We need to init state value. App/src/components/EmojiPicker/EmojiPickerMenu/index.js Lines 80 to 91 in d7ad305
And remove other status change of selectTextOnFocus App/src/components/EmojiPicker/EmojiPickerMenu/index.js Lines 206 to 212 in d7ad305
What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
in componentDidMount What alternative solutions did you explore? (Optional)
|
@sakluger @shawnborton Do you agree this is a bug? Do we want to fix this? |
Hmm I'm not sure if this is even a bug really. But that being said, I'm curious if you have context why we highlight the search term when you blur and then come back? cc @Expensify/design for visibility. |
@shawnborton The context why we highlight the search text is mentioned in my proposal #28379 (comment) |
Hmm I still don't follow - why exactly are we doing this for the search input for the chat switcher? |
I also find it interesting that the behavior is different in each different search context, and for the chat switcher, I'm not really sure why we would highlight the entire field after clicking out and back into it. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@mananjadhav, @sakluger Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
We're still discussing the desired behavior in Slack. |
@mananjadhav, @sakluger Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Still discussing. |
Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@mananjadhav @sakluger @MariaHCD this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks! |
📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($500) |
📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app! |
@mananjadhav PR #29949 is ready for review |
🎯 ⚡️ Woah @mananjadhav / @DylanDylann, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.88-11 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 2023-10-30. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Summarizing payouts for this issue: Reporter: @dhanashree-sawant $50 (paid via Upwork) Above payments include efficiency bonus 🎉 |
Thanks for summary @sakluger. I am not sure if we want to treat this as a bug, as we went through a long discussion before even deciding the expected behavior. The expected result also turned out to be, removing the text selection altogether. That being said I don't think we need a regression test for this one, as we've remove the text focus. @sakluger If there's any existing regression test for the text focus, we would have to remove/alter that. Possible for you to check? |
Sounds good @mananjadhav. I think we can treat it like a bug as far as payments go because we had one inconsistent behavior and we decided to change it to be consistent. But I agree that most of the BZ checklist is a inappropriate - there was no "regression" because the were disagreements around expected behavior. I'm going to close out, please request manual payment for the issue (or reach out to me if you need me to reopen the issue until you make the payment request). |
$750 payment approved for @mananjadhav based on this summary. |
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:
Expected Result:
App should select the search term on refocus to emoji picker search as it does for other searches throughout the appFollowing from this Slack discussion, we should never auto-highlight text upon refocus.
Actual Result:
App auto-highlights text upon refocus in some cases (1- In the Command+K chat switcher when you unfocus and refocus, and 2- in the emoji picker search when you unfocus, press any key to refocus, then unfocus and refocus with the mouse), but not in other cases (in the emoji picker search when unfocusing and refocusing).
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.74-3
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
Notes/Photos/Videos: Any additional supporting documentation
Recording.1664.mp4
windows.chrome.emoji.search.term.not.selected.on.refocus.mp4
android.chrome.emoji.search.term.not.selected.on.refocus.mp4
mac.desktop.mac.chrome.ios.safari.no.full.selection.on.emoji.search.mov
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695750651606789
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: