-
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
Create a common function to insert an emoji into a text #17346
Create a common function to insert an emoji into a text #17346
Conversation
@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
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Check my comment about ReportUtils
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/cead22 in version: 1.3.4-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.4-0 🚀
|
}, | ||
})); | ||
this.updateComment(newComment); | ||
this.updateComment(ComposerUtils.insertText(this.comment, this.state.selection, emoji)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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