-
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
feat: add whitespace after inserting emoji via native keyboard #30412
Changes from all commits
4a67723
6db2014
a8bd005
d219bc6
ec3284d
51406fc
fe60c6f
12273a3
bb6bd37
ad91e78
f00a383
b1213cd
ff7a0ef
681b606
9f3653c
c58c3a3
40a6f85
c66bc8c
f2c1022
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -117,6 +117,7 @@ function ComposerWithSuggestions({ | |||
return draft; | ||||
}); | ||||
const commentRef = useRef(value); | ||||
const lastTextRef = useRef(value); | ||||
|
||||
const {isSmallScreenWidth} = useWindowDimensions(); | ||||
const maxComposerLines = isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES; | ||||
|
@@ -196,6 +197,50 @@ function ComposerWithSuggestions({ | |||
RNTextInputReset.resetKeyboardInput(findNodeHandle(textInputRef.current)); | ||||
}, [textInputRef]); | ||||
|
||||
/** | ||||
* Find the newly added characters between the previous text and the new text based on the selection. | ||||
* | ||||
* @param {string} prevText - The previous text. | ||||
* @param {string} newText - The new text. | ||||
* @returns {object} An object containing information about the newly added characters. | ||||
* @property {number} startIndex - The start index of the newly added characters in the new text. | ||||
* @property {number} endIndex - The end index of the newly added characters in the new text. | ||||
* @property {string} diff - The newly added characters. | ||||
*/ | ||||
const findNewlyAddedChars = useCallback( | ||||
(prevText, newText) => { | ||||
let startIndex = -1; | ||||
let endIndex = -1; | ||||
let currentIndex = 0; | ||||
|
||||
// Find the first character mismatch with newText | ||||
while (currentIndex < newText.length && prevText.charAt(currentIndex) === newText.charAt(currentIndex) && selection.start > currentIndex) { | ||||
currentIndex++; | ||||
} | ||||
|
||||
if (currentIndex < newText.length) { | ||||
startIndex = currentIndex; | ||||
|
||||
// if text is getting pasted over find length of common suffix and subtract it from new text length | ||||
if (selection.end - selection.start > 0) { | ||||
const commonSuffixLength = ComposerUtils.getCommonSuffixLength(prevText, newText); | ||||
endIndex = newText.length - commonSuffixLength; | ||||
} else { | ||||
endIndex = currentIndex + (newText.length - prevText.length); | ||||
} | ||||
} | ||||
|
||||
return { | ||||
startIndex, | ||||
endIndex, | ||||
diff: newText.substring(startIndex, endIndex), | ||||
}; | ||||
}, | ||||
[selection.end, selection.start], | ||||
); | ||||
|
||||
const insertWhiteSpace = (text, index) => `${text.slice(0, index)} ${text.slice(index)}`; | ||||
|
||||
const debouncedSaveReportComment = useMemo( | ||||
() => | ||||
_.debounce((selectedReportID, newComment) => { | ||||
|
@@ -213,7 +258,14 @@ function ComposerWithSuggestions({ | |||
const updateComment = useCallback( | ||||
(commentValue, shouldDebounceSaveComment) => { | ||||
raiseIsScrollLikelyLayoutTriggered(); | ||||
const {text: newComment, emojis} = EmojiUtils.replaceAndExtractEmojis(commentValue, preferredSkinTone, preferredLocale); | ||||
const {startIndex, endIndex, diff} = findNewlyAddedChars(lastTextRef.current, commentValue); | ||||
const isEmojiInserted = diff.length && endIndex > startIndex && EmojiUtils.containsOnlyEmojis(diff); | ||||
const {text: newComment, emojis} = EmojiUtils.replaceAndExtractEmojis( | ||||
isEmojiInserted ? insertWhiteSpace(commentValue, endIndex) : commentValue, | ||||
preferredSkinTone, | ||||
preferredLocale, | ||||
); | ||||
|
||||
if (!_.isEmpty(emojis)) { | ||||
const newEmojis = EmojiUtils.getAddedEmojis(emojis, emojisPresentBefore.current); | ||||
if (!_.isEmpty(newEmojis)) { | ||||
|
@@ -258,13 +310,14 @@ function ComposerWithSuggestions({ | |||
} | ||||
}, | ||||
[ | ||||
debouncedUpdateFrequentlyUsedEmojis, | ||||
preferredLocale, | ||||
raiseIsScrollLikelyLayoutTriggered, | ||||
findNewlyAddedChars, | ||||
preferredSkinTone, | ||||
reportID, | ||||
preferredLocale, | ||||
setIsCommentEmpty, | ||||
debouncedUpdateFrequentlyUsedEmojis, | ||||
suggestionsRef, | ||||
raiseIsScrollLikelyLayoutTriggered, | ||||
reportID, | ||||
debouncedSaveReportComment, | ||||
], | ||||
); | ||||
|
@@ -315,14 +368,8 @@ function ComposerWithSuggestions({ | |||
* @param {Boolean} shouldAddTrailSpace | ||||
*/ | ||||
const replaceSelectionWithText = useCallback( | ||||
(text, shouldAddTrailSpace = true) => { | ||||
const updatedText = shouldAddTrailSpace ? `${text} ` : text; | ||||
const selectionSpaceLength = shouldAddTrailSpace ? CONST.SPACE_LENGTH : 0; | ||||
updateComment(ComposerUtils.insertText(commentRef.current, selection, updatedText)); | ||||
setSelection((prevSelection) => ({ | ||||
start: prevSelection.start + text.length + selectionSpaceLength, | ||||
end: prevSelection.start + text.length + selectionSpaceLength, | ||||
})); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this removed code makes a regression. Screen.Recording.2023-10-30.at.20.43.23.movStep to reproduce:
The cursor should stay in the last position before the unfocus. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch π. I had inadvertently removed the code to reset cursor position when the composer was focused again. However the method |
||||
(text) => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to update these line
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Removed the second parameter which is redundant now. |
||||
updateComment(ComposerUtils.insertText(commentRef.current, selection, text)); | ||||
}, | ||||
[selection, updateComment], | ||||
); | ||||
|
@@ -446,7 +493,12 @@ function ComposerWithSuggestions({ | |||
} | ||||
|
||||
focus(); | ||||
replaceSelectionWithText(e.key, false); | ||||
// Reset cursor to last known location | ||||
setSelection((prevSelection) => ({ | ||||
start: prevSelection.start + 1, | ||||
end: prevSelection.end + 1, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused about this logic. Previously, we also sum the text length, but not here. @aswin-s Could you explain more? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mollfpr Earlier it had to handle both Emoji picker input and Keyboard onPress. When emoji was inserted via picker selection, it had to adjust for emoji with white space. Now since the logic is moved into key press event, the selection is always going to be incremented by one, since it gets triggered only once for the first character being inserted. |
||||
})); | ||||
replaceSelectionWithText(e.key); | ||||
}, | ||||
[checkComposerVisibility, focus, replaceSelectionWithText], | ||||
); | ||||
|
@@ -510,6 +562,10 @@ function ComposerWithSuggestions({ | |||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||||
}, []); | ||||
|
||||
useEffect(() => { | ||||
lastTextRef.current = value; | ||||
}, [value]); | ||||
|
||||
useImperativeHandle( | ||||
forwardedRef, | ||||
() => ({ | ||||
|
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.
Missing the params JSDoc.
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.
Added JSDoc params