-
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
Static emoji autosuggestion #14686
Static emoji autosuggestion #14686
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@Beamanator Please 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] |
I'll review this first thing on Monday |
This comment was marked as resolved.
This comment was marked as resolved.
I have read the CLA Document and I hereby sign the CLA |
@perunt This PR will still be unmergable because it contains these older unsigned commits: |
da3e291
to
fc4b0c9
Compare
@perunt this commit will be a problem (gpg signature needs to be verified by GitHub): |
ae3f1b7
to
69ec073
Compare
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.
Overall this is looking much better to me
const newComment = this.comment.slice(0, this.state.selection.start) | ||
+ emoji + this.comment.slice(this.state.selection.end, this.comment.length); | ||
+ emojiWithSpace |
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.
It does not look like this change was made. Maybe you need to disable prettier?
@mananjadhav tagging you for review since you're the C+ on the linked issue: #12188 |
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issue in the future. |
Hi Everyone we missed handling a case here which lead to this issue #16626. IMO we could have caught this if considered testing other possible scenarios. |
hovered, | ||
index, | ||
)} | ||
onMouseDown={e => e.preventDefault()} |
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! I'm coming from #16485
onMouseDown={e => e.preventDefault()}
prevents the focus from moving and the keyboard from closing. But unfortunately, when we long press the emoji element, the mousedown event is not fired on mWeb Android.
Causing the bug - #16485
The mWeb Android behavior is well demonstrated here.
Hello everyone, Looks like we missed to handle this issue while implementing the feature. Issue: #16977
|
Hello everyone this PR caused a console error. More details here |
HIYA! Coming from #18793 This PR causes a crash upon searching the mention back to back; complete step here.
|
const commentBeforeColon = this.state.value.slice(0, this.state.colonIndex); | ||
const emojiObject = this.state.suggestedEmojis[highlightedEmojiIndex]; | ||
const emojiCode = emojiObject.types && emojiObject.types[this.props.preferredSkinTone] ? emojiObject.types[this.props.preferredSkinTone] : emojiObject.code; | ||
const commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end).replace(CONST.REGEX.EMOJI_REPLACER, CONST.SPACE); |
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.
@@ -1003,6 +1021,8 @@ const CONST = { | |||
CHAT_ATTACHMENT_TOKEN_KEY: 'X-Chat-Attachment-Token', | |||
|
|||
USA_COUNTRY_NAME, | |||
SPACE_LENGTH: 1, | |||
SPACE: 1, |
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 caused a regression #19289. In our world SPACE is still
and not 1
😅.
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.
whoooops 😅
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.
😄
Coming from #18112 In the above issue, on Android scrolling on the emoji suggestion will scroll the report action list.
|
nextState.shouldShowSuggestionMenu = !_.isEmpty(newSuggestedEmojis); | ||
} | ||
|
||
LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity)); |
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.
Why did we add this LayoutAnimation @perunt?
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.
It was added to show a suggestion box. I just checked the code and noticed we introduced an animatedValue to control it. So, I think we can just remove LayoutAnimation
emojiSuggestionsText: { | ||
fontFamily: fontFamily.EMOJI_TEXT_FONT, | ||
fontSize: variables.fontSizeMedium, | ||
}, |
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 caused an issue when emoji text became too long. We should have allowed word breaking here.
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.
thanks for let us know.
@@ -582,6 +704,7 @@ class ReportActionCompose extends React.Component { | |||
<TouchableOpacity | |||
onPress={(e) => { | |||
e.preventDefault(); | |||
this.setShouldShowSuggestionMenuToFalse(); |
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.
Why did we close the suggestions on Expand/Collapse cc: @mananjadhav @perunt?
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.
Would love to hear your opinion @mananjadhav @perunt.
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.
It's tough to remember all the details, but the first thing that comes to mind is that we adopted this feature from other messengers. We can definitely keep it displayed
const prefixLocation = name.search(prefix); | ||
|
||
if (prefixLocation === 0 && prefix.length === name.length) { | ||
texts.push({text: prefix, isColored: true}); |
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.
text should be name
instead of prefix
.
Later, this caused minor regression - #32749
Details
Sets up an emojicode autosuggestion UI.
Fixed Issues
$ #12188
Tests
Offline tests
QA Steps
Condition: You are in the chat screen and the text input is focused with the keyboard expanded
Mobile:
Enter
:s
in an empty text input.Observe that there is no suggestion.
Add 'm' to the text input so it contains
:sm
.Verify that the Emoji suggester appears above the text input.
Tap on any of the suggested emojis.
Verify that
:sm
is replaced with the selected emoji.Enter some text in the text input.
Add a space or create a new line.
Expand the input
Enter a colon followed by two letters that correspond to the name of the desired emoji (e.g. smile ->
:sm
, flag ->:fl
).Verify that the Emoji suggester appears above the text input.
Tap on one of the suggested emojis.
Verify that the text (e.g.
:sm
,:fl
, etc.) is replaced with the selected emoji.WEB/Desktop:
:s
in an empty text input.:sm
.:sm
is replaced with the selected emoji.Note: The suggestion will only appear if you enter a colon followed by two letters without any spaces in between. Before the colon, there should be either a space, a line break, or the input should be the first word in the text field. (So, the following values would not trigger the suggestion:
:s
,: sm
,:s m
,sm:sm
)Emoji suggestions could contain up to 5 items. Is a screen small we show a height of 2.5 items and you can scroll this box.
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.Screenshots/Videos
Web
Jan-31-2023.22-22-38.mp4
Mobile Web - Chrome
Feb-12-2023.19-06-12.mp4
Mobile Web - Safari
Feb-12-2023.19-06-34.mp4
Desktop
Jan-31-2023.22-28-10.mp4
iOS
Jan-31-2023.22-02-45.mp4
Android
Jan-31-2023.22-13-56.mp4