-
Notifications
You must be signed in to change notification settings - Fork 3k
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 2024-02-07] [$2000] Chat - Whitespace is not added after an emoji while inserting via native keyboard #23658
Comments
Triggered auto assignment to @CortneyOfstad ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Whitespace is not added after an emoji while inserting via native keyboard What is the root cause of that problem?No logic has been implemented for automatically adding whitespace after inserting an emoji via the native keyboard. What changes do you think we should make in order to solve the problem?In
- onChangeText={(comment) => this.updateComment(comment, true)}
+ onChange={this.updateCommentAndProcessEmoji}
updateCommentAndProcessEmoji(event) {
if (event.nativeEvent.data && event.nativeEvent.isComposing && EmojiUtils.containsOnlyEmojis(event.nativeEvent.data)) {
this.replaceSelectionWithText(event.nativeEvent.data);
} else {
this.updateComment(event.nativeEvent.text, true);
}
}
This change would also be applied to What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Experience of inserting emoji in chat is not unified! What is the root cause of that problem?So I will open a new discussion in slack. @mallenexpensify proposed the space but this is only true when emoji is written with :emoji, when the emoji is picked from the picker menu in slack it is written without trail space. We would have to write custom code to "process" the written emoji and set a space in the What changes do you think we should make in order to solve the problem?I would remove the trail from Video of solution working same as slack with native menu and EmojiPickerMenu: working.as.slack.mp4What alternative solutions did you explore? (Optional)Update the |
Job added to Upwork: https://www.upwork.com/jobs/~0188d7c4a5e13254b6 |
Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
@daordonez11 I still prefer consistency in our emoji addition, and having a space is what we have now. So we should fix it by adding an emoji from the native keyboard.
@samh-nl What's your plan for this? Will we replace the current |
Oh yeah I agree @mollfpr it was discussed in this thread https://expensify.slack.com/archives/C01GTK53T8Q/p1690483337635379 definitely the other proposal is the way to go. I'll check if there's a better proposal to be done. |
I'm heading OoO until 8/14, so reassigning BZ to keep an eye on this while I'm out 👍 |
Triggered auto assignment to @zanyrenney ( |
Bug0 Triage Checklist (Main S/O)
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Friendly bump @samh-nl |
@mollfpr bump ^^^ Thanks! |
@mollfpr bump again — TIA! |
Sorry for the delay 🙏 I'm starting to work on this issue again and testing the new PR. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.33-5 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 2024-02-07. 🎊 For reference, here are some details about the assignees on this 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:
|
New feature.
@CortneyOfstad Could you give the payment summary for manual request? Thank you! |
@CortneyOfstad The issue was reported before the bounty for issues were reduced to 50$. |
Regression test listed here 👍 |
Sorry about that @aswin-s! I adjusted the payment to be $250, and will update the payment summary above. You should have received both payments successfully, totaling $2,250 👍 |
Shoot — that does reduce the payment and it was already completed 🙃 Going to post in Open-Source on how to get this corrected. |
Alright, @aswin-s I requested a refund of 50% ($1000) as the contributor. Please let me know as soon as you complete this. Thank you! |
@CortneyOfstad Let me know if I can do anything from my end |
Thank you |
Thank you so much @aswin-s — I greatly appreciate it! |
@CortneyOfstad @situchan Quick question. The regression was caught during staging itself and fix was raised the very next day. So payment would be deducted even if developer fixes it even before it hits PROD? Just asking so that I can be more careful next time. |
@aswin-s Thanks for asking! Each error in proposed PRs will carry different weights based on the type of error and the impact it would have, so in some cases (like this one) yes, it will cause a reduction in payment. |
There are some exceptions. i.e. regressions penalties won't apply if already discussed as out of scope in PR. |
$1,000 approved for @mollfpr based on this summary. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
As per the issue #11100 and slack discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1662746279291419 a white space is expected after each emoji.
Actual Result:
Whitespace is added only if emoji is added via emoji picker in composer and not while added using native keyboard.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.44-2
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
emoji.webm
RPReplay_Final1690382962.1.MP4
Expensify/Expensify Issue URL:
Issue reported by: @aswin-s
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690359926207719
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: