-
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 15 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,43 @@ function ComposerWithSuggestions({ | |||
RNTextInputReset.resetKeyboardInput(findNodeHandle(textInputRef.current)); | ||||
}, [textInputRef]); | ||||
|
||||
/** | ||||
* Find diff between text changes in composer | ||||
*/ | ||||
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 +251,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 +303,14 @@ function ComposerWithSuggestions({ | |||
} | ||||
}, | ||||
[ | ||||
debouncedUpdateFrequentlyUsedEmojis, | ||||
preferredLocale, | ||||
raiseIsScrollLikelyLayoutTriggered, | ||||
findNewlyAddedChars, | ||||
preferredSkinTone, | ||||
reportID, | ||||
preferredLocale, | ||||
setIsCommentEmpty, | ||||
debouncedUpdateFrequentlyUsedEmojis, | ||||
suggestionsRef, | ||||
raiseIsScrollLikelyLayoutTriggered, | ||||
reportID, | ||||
debouncedSaveReportComment, | ||||
], | ||||
); | ||||
|
@@ -315,14 +361,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], | ||||
); | ||||
|
@@ -510,6 +550,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