Create a common function to insert an emoji into a text #17346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
melvin-bot
bot
requested review from
cead22 and
Santhosh-Sellavel
and removed request for
a team
|
@cead22 @Santhosh-Sellavel One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
PR is ready @Santhosh-Sellavel |
|
Conflicts |
|
@cead22 Conflict with this PR #17138. The logic of App/src/pages/home/report/ReportActionCompose/index.js Lines 504 to 514 in 42bea4b 1st App/src/pages/home/report/ReportActionCompose/index.js Lines 521 to 524 in 42bea4b 2nd Is it still okay to use |
cead22
changed the title
|
That would look weird. Definelty |
This reverts commit aeb0286.
|
I moved the function to |
|
I agree The most fitting *Utils exiting file seems to be |
|
Makes sense @cead22 |
cead22
reviewed
Apr 14, 2023
|
Agree that diff --git a/src/components/Composer/index.js b/src/components/Composer/index.js
index 9b2d028a73..fe192de230 100755
--- a/src/components/Composer/index.js
+++ b/src/components/Composer/index.js
@@ -9,7 +9,7 @@ import withLocalize, {withLocalizePropTypes} from '../withLocalize';
import Growl from '../../libs/Growl';
import themeColors from '../../styles/themes/default';
import updateIsFullComposerAvailable from '../../libs/ComposerUtils/updateIsFullComposerAvailable';
-import getNumberOfLines from '../../libs/ComposerUtils/index';
+import * as ComposerUtils from '../../libs/ComposerUtils/index';
import * as Browser from '../../libs/Browser';
import Clipboard from '../../libs/Clipboard';
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';
@@ -343,7 +343,7 @@ class Composer extends React.Component {
const lineHeight = parseInt(computedStyle.lineHeight, 10) || 20;
const paddingTopAndBottom = parseInt(computedStyle.paddingBottom, 10)
+ parseInt(computedStyle.paddingTop, 10);
- const computedNumberOfLines = getNumberOfLines(this.props.maxLines, lineHeight, paddingTopAndBottom, this.textInput.scrollHeight);
+ const computedNumberOfLines = ComposerUtils.getNumberOfLines(this.props.maxLines, lineHeight, paddingTopAndBottom, this.textInput.scrollHeight);
const numberOfLines = computedNumberOfLines === 0 ? this.props.numberOfLines : computedNumberOfLines;
updateIsFullComposerAvailable(this.props, numberOfLines);
this.setState({
diff --git a/src/libs/ComposerUtils/index.js b/src/libs/ComposerUtils/index.js
index a469da7516..e207f2bcaf 100644
--- a/src/libs/ComposerUtils/index.js
+++ b/src/libs/ComposerUtils/index.js
@@ -14,4 +14,18 @@ function getNumberOfLines(maxLines, lineHeight, paddingTopAndBottom, scrollHeigh
return newNumberOfLines;
}
-export default getNumberOfLines;
+/**
+ * Replace substring between selection with a text.
+ * @param {String} text
+ * @param {Object} selection
+ * @param {String} textToInsert
+ * @returns {String}
+ */
+function insertText(text, selection, textToInsert) {
+ return text.slice(0, selection.start) + textToInsert + text.slice(selection.end, text.length);
+}
+
+export {
+ getNumberOfLines,
+ insertText,
+};
diff --git a/src/libs/ComposerUtils/index.native.js b/src/libs/ComposerUtils/index.native.js
index 783fbac9a4..82c0d5191a 100644
--- a/src/libs/ComposerUtils/index.native.js
+++ b/src/libs/ComposerUtils/index.native.js
@@ -1,6 +1,7 @@
import lodashGet from 'lodash/get';
import styles from '../../styles/styles';
import updateIsFullComposerAvailable from './updateIsFullComposerAvailable';
+import * as ComposerUtils from './index.js';
/**
* Get the current number of lines in the composer
@@ -32,7 +33,17 @@ function updateNumberOfLines(props, e) {
updateIsFullComposerAvailable(props, numberOfLines);
}
+/**
+ * Replace substring between selection with a text.
+ * @param {String} text
+ * @param {Object} selection
+ * @param {String} textToInsert
+ * @returns {String}
+ */
+const insertText = ComposerUtils.insertText;
+
export {
getNumberOfLines,
updateNumberOfLines,
+ insertText,
};
diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js
index 4eb3e9b4d5..f35f2ceb77 100644
--- a/src/libs/ReportUtils.js
+++ b/src/libs/ReportUtils.js
@@ -1707,17 +1707,6 @@ function canLeaveRoom(report, isPolicyMember) {
return true;
}
-/**
- * Replace substring between selection with a text.
- * @param {String} text
- * @param {Object} selection
- * @param {String} textToInsert
- * @returns {String}
- */
-function insertText(text, selection, textToInsert) {
- return text.slice(0, selection.start) + textToInsert + text.slice(selection.end, text.length);
-}
-
export {
getReportParticipantsTitle,
isReportMessageAttachment,
@@ -1786,5 +1775,4 @@ export {
getSmallSizeAvatar,
getMoneyRequestOptions,
canRequestMoney,
- insertText,
};
diff --git a/src/pages/home/report/ReportActionCompose/index.js b/src/pages/home/report/ReportActionCompose/index.js
index 1368305347..e5190bd9a3 100644
--- a/src/pages/home/report/ReportActionCompose/index.js
+++ b/src/pages/home/report/ReportActionCompose/index.js
@@ -52,6 +52,7 @@ import withKeyboardState, {keyboardStatePropTypes} from '../../../../components/
import ArrowKeyFocusManager from '../../../../components/ArrowKeyFocusManager';
import KeyboardShortcut from '../../../../libs/KeyboardShortcut';
import KeyDownAction from './keyDownAction';
+import * as ComposerUtils from '../../../../libs/ComposerUtils';
const propTypes = {
/** Beta features list */
@@ -508,7 +509,7 @@ class ReportActionCompose extends React.Component {
end: prevState.selection.start + text.length,
},
}));
- this.updateComment(ReportUtils.insertText(this.comment, this.state.selection, text));
+ this.updateComment(ComposerUtils.insertText(this.comment, this.state.selection, text));
}
/**
diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index fc97e34f16..95f4caa724 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -27,6 +27,7 @@ import withWindowDimensions, {windowDimensionsPropTypes} from '../../../componen
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import withKeyboardState, {keyboardStatePropTypes} from '../../../components/withKeyboardState';
import ONYXKEYS from '../../../ONYXKEYS';
+import * as ComposerUtils from '../../../libs/ComposerUtils';
const propTypes = {
/** All the data of the action */
@@ -235,7 +236,7 @@ class ReportActionItemMessageEdit extends React.Component {
end: prevState.selection.start + emoji.length,
},
}));
- this.updateDraft(ReportUtils.insertText(this.state.draft, this.state.selection, emoji));
+ this.updateDraft(ComposerUtils.insertText(this.state.draft, this.state.selection, emoji));
}
/**
@cead22 @Santhosh-Sellavel what do you think? |
|
That works |
|
Updated. |
|
Solved. |
cead22
previously approved these changes
Apr 18, 2023
Santhosh-Sellavel
suggested changes
Apr 19, 2023
|
Please resolve conflicts |
|
Solved. |
|
Conflicts again 🥲 |
|
|
Reviewer Checklist
Screenshots/VideosWeb & DesktopScreen.Recording.2023-04-21.at.1.13.38.AM.movMobile Web - Chrome & Mobile Web - SafariScreen.Recording.2023-04-21.at.1.30.30.AM.moviOS & AndroidScreen.Recording.2023-04-21.at.1.27.14.AM.mov |
|
@cead22 Looks good to me just one query cc: @bernhardoj |
cead22
approved these changes
Apr 20, 2023
There was a problem hiding this comment.
@Santhosh-Sellavel does the file/dir structure look good to you?
|
Yes, looks good to me! |
@cead22 Whats expected in this case? |
There was a problem hiding this comment.
Tests are passing & code LGTM!
Only one query, If the space is expected to add while adding emoji via shorthand we are good to go other wise need to fix that here, thanks!
cc: @bernhardoj @cead22
|
@Santhosh-Sellavel Based on the slack discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1668009281985659?thread_ts=1667208968.300839&cid=C01GTK53T8Q, if we add emoji by |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
14 tasks
|
🚀 Deployed to staging by https://github.com/cead22 in version: 1.3.4-0 🚀
|
6 tasks
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.4-0 🚀
|
6 tasks
mollfpr
reviewed
May 25, 2023
| }, | ||
| })); | ||
| this.updateComment(newComment); | ||
| this.updateComment(ComposerUtils.insertText(this.comment, this.state.selection, emoji)); |
There was a problem hiding this comment.
Hi, coming from #18515. This PR raise regression Emoji gets added in between the text when selecting text and adding new emoji by search and entering.
The this.state.selection we pass should be the state before it gets updated, it's due to the state update may be asynchronous.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Currently, a space is added when we add an emoji from emoji picker to main composer. The expected behavior is to not add a space and this PR address it.
Fixed Issues
$ #15951
PROPOSAL: #15951 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
android.mweb.mov
Mobile Web - Safari
ios.mweb.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mp4