fix: ComposeBox - When you click on the mention again, extra characters appear. - #63843
Conversation
…rs appear. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@hoangzinh, I was recording the videos, but unfortunately, I found one edge case that I need to resolve. I’ll provide updates again as soon as I find a solution for it. Monosnap.screencast.2025-06-11.23-09-59.mp4 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@hoangzinh 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] |
|
@hoangzinh, this is ready for review. Please take some time to test it thoroughly—there might be some edge cases I missed during my testing, though I tried to cover all possible scenarios. |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| return (str.match(/ /g) || []).length; |
There was a problem hiding this comment.
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| return (str.match(/ /g) || []).length; | |
| return (str.match(/ /g) ?? []).length; |
Is there any case that we need to use ||?
There was a problem hiding this comment.
updated to "??".
| if (whiteSpacesLength) { | ||
| const str = rest.split(' ', whiteSpacesLength + 1).join(' '); | ||
| return rest.slice(0, str.length); | ||
| } | ||
|
|
||
| const breakerIndex = rest.search(CONST.REGEX.MENTION_BREAKER); | ||
| return breakerIndex === -1 ? rest : rest.slice(0, breakerIndex); |
There was a problem hiding this comment.
Can you add comments for that logic, please?, it's kind of tricky
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 can you resolve conflict? |
|
@Krishna2323 can you resoilve conflict please? |
|
@hoangzinh, sorry for delay, I'm on a trek and don't have my laptop RN. I'll resolve conflicts by EOD tomorrow. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
16dab86 to
5c78f1a
Compare
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-06-23.at.18.23.22.android.movAndroid: mWeb ChromeScreen.Recording.2025-06-23.at.18.25.39.android.chrome.moviOS: HybridAppScreen.Recording.2025-06-23.at.18.28.12.moviOS: mWeb SafariScreen.Recording.2025-06-23.at.18.29.45.movMacOS: Chrome / SafariScreen.Recording.2025-06-23.at.18.18.34.web.movMacOS: DesktopScreen.Recording.2025-06-23.at.18.26.27.desktop.mov |
dangrous
left a comment
There was a problem hiding this comment.
Okay, this is looking good! A couple questions:
- Have we tested this against all of the deploy blockers that required revert?
- Do we need to count all whitespace characters (
\s) or just single spaces? If it's only single spaces, let's update the names tocountSpacesinstead ofcountWhiteSpacesetc. But, if we need to count all whitespace, we need to update thestr.matchon L156 to/\s/g
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@dangrous, both deploy blockers have been tested and we need to count all whitespace, I have updated |
Yes, @dangrous. They're included in testing steps of this PR. |
There was a problem hiding this comment.
Thanks! This LGTM - do you want to give one more quick test after the update, @hoangzinh, or should we be set?
|
I'm going to go ahead and merge, I think we should be good here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
oh I missed your message. Yes, hoping |
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.1.72-0 🚀
|
|
🙏 |
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.1.72-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.1.72-10 🚀
|
Explanation of Change
Fixed Issues
$ #60804
PROPOSAL: #60804 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4