[HOLD for payment 2023-01-11] Web/Desktop - Emoji Picker - Space is added after the emoji in compose box #13343
Comments
isagoico
added
Engineering
Daily
|
Triggered auto assignment to @alexpensify ( |
|
Triggered auto assignment to @youssef-lr ( |
|
Creating this issue coming from this comment - https://github.com/Expensify/Expensify/issues/245551#issuecomment-1337440659 |
|
Not a bug, this is actually intended. Lines 202 to 206 in 2e69dac |
|
@s77rt in the note above (which maybe can't be accessed given it's in a diff repo!) we are wanting to have the behavior add a space for emojis ending with a |
ProposalIf we don't want the space when choosing emoji from the picker we just have to make these changes - At message composer addEmojiToTextBox(emoji) {
- const emojiWithSpace = `${emoji} `;
const newComment = this.comment.slice(0, this.state.selection.start)
- + emojiWithSpace + this.comment.slice(this.state.selection.end, this.comment.length);
+ + emoji + this.comment.slice(this.state.selection.end, this.comment.length);
+ this.textInput.setNativeProps({
+ text: newComment,
+ });
this.setState(prevState => ({
selection: {
- start: prevState.selection.start + emojiWithSpace.length,
- end: prevState.selection.start + emojiWithSpace.length,
+ start: prevState.selection.start + emoji.length,
+ end: prevState.selection.start + emoji.length,
},At edit composer addEmojiToTextBox(emoji) {
- const emojiWithSpace = `${emoji} `;
const newComment = this.state.draft.slice(0, this.state.selection.start)
- + emojiWithSpace + this.state.draft.slice(this.state.selection.end, this.state.draft.length);
+ + emoji + this.state.draft.slice(this.state.selection.end, this.state.draft.length);
this.setState(prevState => ({
selection: {
- start: prevState.selection.start + emojiWithSpace.length,
- end: prevState.selection.start + emojiWithSpace.length,
+ start: prevState.selection.start + emoji.length,
+ end: prevState.selection.start + emoji.length,
}, |
ProposalIn order to add space just on mobile, we can make the following changes: diff --git a/src/libs/EmojiUtils.js b/src/libs/EmojiUtils.js
index 617c624b2..bd2a76868 100644
--- a/src/libs/EmojiUtils.js
+++ b/src/libs/EmojiUtils.js
@@ -186,9 +186,10 @@ function addToFrequentlyUsedEmojis(frequentlyUsedEmojis, newEmoji) {
/**
* Replace any emoji name in a text with the emoji icon
* @param {String} text
+ * @param {Boolean} addSpace
* @returns {String}
*/
-function replaceEmojis(text) {
+function replaceEmojis(text, addSpace = true) {
let newText = text;
const emojiData = text.match(CONST.REGEX.EMOJI_NAME);
if (!emojiData || emojiData.length === 0) {
@@ -201,7 +202,7 @@ function replaceEmojis(text) {
// If this is the last emoji in the message and it's the end of the message so far,
// add a space after it so the user can keep typing easily.
- if (i === emojiData.length - 1 && text.endsWith(emojiData[i])) {
+ if (addSpace && i === emojiData.length - 1 && text.endsWith(emojiData[i])) {
emojiReplacement += ' ';
}
newText = newText.replace(emojiData[i], emojiReplacement);
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 37e5602ca..d49b2464f 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -378,7 +378,7 @@ class ReportActionCompose extends React.Component {
* @param {Boolean} shouldDebounceSaveComment
*/
updateComment(comment, shouldDebounceSaveComment) {
- const newComment = EmojiUtils.replaceEmojis(comment);
+ const newComment = EmojiUtils.replaceEmojis(comment, this.props.isSmallScreenWidth);
this.setState((prevState) => {
const newState = {
isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 2f7fbc29d..5f772d84d 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -101,7 +101,7 @@ class ReportActionItemMessageEdit extends React.Component {
* @param {String} draft
*/
updateDraft(draft) {
- const newDraft = EmojiUtils.replaceEmojis(draft);
+ const newDraft = EmojiUtils.replaceEmojis(draft, this.props.isSmallScreenWidth);
this.setState((prevState) => {
const newState = {draft: newDraft};
if (draft !== newDraft) { |
|
@dylanexpensify Why on mobile only? Lines 202 to 206 in 2e69dac |
Proposaldiff --git a/src/libs/EmojiUtils.js b/src/libs/EmojiUtils.js
index 617c624b2c..2e96216176 100644
--- a/src/libs/EmojiUtils.js
+++ b/src/libs/EmojiUtils.js
@@ -198,12 +198,6 @@ function replaceEmojis(text) {
const checkEmoji = emojisTrie.search(emojiData[i].slice(1, -1));
if (checkEmoji && checkEmoji.metaData.code) {
let emojiReplacement = checkEmoji.metaData.code;
-
- // If this is the last emoji in the message and it's the end of the message so far,
- // add a space after it so the user can keep typing easily.
- if (i === emojiData.length - 1 && text.endsWith(emojiData[i])) {
- emojiReplacement += ' ';
- }
newText = newText.replace(emojiData[i], emojiReplacement);
}
}
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 9eb52bbe28..751dab6445 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -323,13 +323,12 @@ class ReportActionCompose extends React.Component {
* @param {String} emoji
*/
addEmojiToTextBox(emoji) {
- const emojiWithSpace = `${emoji} `;
const newComment = this.comment.slice(0, this.state.selection.start)
- + emojiWithSpace + this.comment.slice(this.state.selection.end, this.comment.length);
+ + emoji + this.comment.slice(this.state.selection.end, this.comment.length);
this.setState(prevState => ({
selection: {
- start: prevState.selection.start + emojiWithSpace.length,
- end: prevState.selection.start + emojiWithSpace.length,
+ start: prevState.selection.start + emoji.length,
+ end: prevState.selection.start + emoji.length,
},
}));
this.updateComment(newComment);
diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 2f7fbc29db..960d4a4c9b 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -177,13 +177,12 @@ class ReportActionItemMessageEdit extends React.Component {
* @param {String} emoji
*/
addEmojiToTextBox(emoji) {
- const emojiWithSpace = `${emoji} `;
const newComment = this.state.draft.slice(0, this.state.selection.start)
- + emojiWithSpace + this.state.draft.slice(this.state.selection.end, this.state.draft.length);
+ + emoji + this.state.draft.slice(this.state.selection.end, this.state.draft.length);
this.setState(prevState => ({
selection: {
- start: prevState.selection.start + emojiWithSpace.length,
- end: prevState.selection.start + emojiWithSpace.length,
+ start: prevState.selection.start + emoji.length,
+ end: prevState.selection.start + emoji.length,
},
}));
this.updateDraft(newComment);DetailsI removed the extra space since it's unwanted and because as a user I would not except to get any extra spaces. Using my keyboard emoji picker does not add spaces, why would the app's emoji picker add spaces? |
|
to clarify, adding the space for mobile, but not desktop/web is an internal design decision we made @s77rt |
|
@dylanexpensify I understand, in this case maybe you should reconsider that decision as I can't see any benefit from adding space after an emoji, if anything it's an unexpected behaviour from user perspective |
|
Hi @s77rt appreciate your concern, but we're going to move forward with the decision as it is. |
|
@youssef-lr - Can you confirm if we should add the |
|
@dylanexpensify @alexpensify I'll grab this. |
alexpensify
added
the
Internal
|
Thanks, @youssef-lr! Keep us posted if you need me to test when the fix is ready. |
|
@youssef-lr - I'm checking in to see how things are going here. |
|
No update yet, I'm going OOO until January 3 and will catch up then to identify if there is any movement here. |
|
For anyone on this thread, this was the Slack thread where we landed on this behavior. Please participate there if you disagree where we landed. This is also largely consistent with how Slack handles it. |
|
In other news, given that this bug will hit the 30 day mark by next week, I'm going to take this over from @alexpensify so we can keep it moving this week and get ahead of this passing that mark. cc @youssef-lr |
|
@youssef-lr I saw you weren't feeling well, and I believe that you're headed OOO soon. Will you be able to circle back on this one earlier next week? I'm mindful of staying ahead of the 30 day mark on this one. I'm happy to help find a volunteer if you're not around. |
|
Huh, it looks like this issue was never updated automatically and the regression period has already past. Let's get this one squared away. Here's how I see this one shaking out.
|
JmillsExpensify
added
Bug
|
Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new. |
|
Well that didn't work. I'll just create the Upwork job manually. Here it is: https://www.upwork.com/jobs/~01da97f6e1c8796152. Can both of you apply so we can get this paid out tomorrow? |
JmillsExpensify
changed the title
|
@JmillsExpensify Applied. |
|
Applied, thanks! @JmillsExpensify |
|
Hello. Thanks for your job posting. I am a full stack developer who has much experience with web development related emoji. Please take a look at https://nolanlawson.github.io/emoji-picker-element. I think this project matches with your project very well. I am ready your job. Hope my experience will be helpful for your project. Look forward to discussing more detail about this opportunity. |
|
@allroundexperts @Puneet-here I've extended offers to both of you. |
|
Also for clarity, this was technically a new feature in the name of consistency, so no regressions. |
|
Accepted.
|
|
So I want to know your skype id or mail address. Can you send me them to shinydev1108@gmail.com? |
|
Just waiting on @Puneet-here at this point and then I'll close out the issue. |
|
Sorry for the delay. Accepted now! |
JmillsExpensify
removed
the
Bug
|
Thanks @Puneet-here! You're all set now. |
|
Hey @JmillsExpensify, sorry for the late comment but C+ compensation is due. (Reviewed PR) |
|
Oh, thanks! We need a better process here, probably something you should bring up in C+ in Slack. One idea would be that C+ is responsible for commenting in the issue linked to the PR to ensure that they get assigned and paid. In any case, let's improve our process here, as I notice this keeps happening. |
|
In any case, I invited you to the job as it's still open. Can you please accept? Appreciated! |
|
Applied, thanks @JmillsExpensify |
|
Offer sent. |
|
@thesahindia Before I issue payment, I wanted to clarify the payment. You were assigned as C+ to review this PR, though you didn't review any proposals in this issue. As a result, I think the most fair is to treat this like any other issue where you're just assigned to review a PR. That's $1,000. Does that make sense and do you agree? |
|
Yeah, it makes sense to me. |
|
Great thank you! You should be all set now. |
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:
The emoji should be added in the compose box without any space after the emoji
Actual Result:
Emoji is added with a space in the compose box. On desktop/web, no space should be added after adding a emoji in compose box.
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platform:
Where is this issue occurring?
Version Number: v1.2.36-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers): N/A
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: N/A
Issue reported by: Applause
Slack conversation: N/A
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: