Fix chat doesn't scroll to bottom when adding message while linked to an old action #46724
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
melvin-bot
bot
requested review from
ZhenjaHorbach
and removed request for
a team
|
@ZhenjaHorbach 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] |
|
Send button is still broken on mWeb. ios.mweb.mp4 |
|
Sometimes the scroll to the bottom doesn't work especially if you go to the linked action and then refresh. That will be fixed by #45004. |
Thanks for the information ! |
Reviewer Checklist
Screenshots/VideosAndroid: Native2024-08-12.12.21.01.movAndroid: mWeb Chromeandroid-web.moviOS: Nativeios.moviOS: mWeb Safariios-web.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
| return; | ||
| } | ||
| if (!hasNewestReportActionRef.current) { | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID)); | ||
| return; |
There was a problem hiding this comment.
Do we need return here ?
Because of this line the main issue will still be relevant when we click on the "From" link in header to navigate back to the main chat
And then write something
There was a problem hiding this comment.
I'm trying to follow the same logic of scrollToBottomAndMarkReportAsRead.
App/src/pages/home/report/ReportActionsList.tsx
Lines 415 to 421 in 55a29dd
I guess we need to hold for #46627 then
There was a problem hiding this comment.
Hmmm, yeah
Just tested the changes from another PR with new changes from this PR
And it helped to fix our main issue
Then let's wait until another PR is merged
There was a problem hiding this comment.
@bernhardoj
Another PR was merged !
Could you update the branch to the latest main version, please?
And I think we should update the description and add new videos for this PR
Because first of all we should fix the bug in the issue description
There was a problem hiding this comment.
Done (except native because pressing the send button crashes the iOS and doesn't work on Android, on mWeb disabling strict mode makes it work again).
ios.mp4
There was a problem hiding this comment.
I'm more talking about why scrolling doesn't work on Android native when go back to the main chat and send message
There was a problem hiding this comment.
Oh, just reinstalled android app
And everything works
Could you please reinstall ios and android native apps and record videos for checklist?
There was a problem hiding this comment.
Nice, it works. Completed recordings
There was a problem hiding this comment.
Great job
Thank you !
bernhardoj
changed the title
bernhardoj
changed the title
|
LGTM |
ZhenjaHorbach
approved these changes
Aug 12, 2024
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
77 tasks
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.0.22-0 🚀
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.0.22-1 🚀
|
6 tasks
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.22-9 🚀
|
suneox
reviewed
Oct 25, 2024
| return; | ||
| } | ||
| if (!hasNewestReportActionRef.current) { | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID)); |
There was a problem hiding this comment.
We are missing handling the case when the chat is on RHN, which caused this issue with the report on RHN navigating to the center.
6 tasks
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
The chat doesn't scroll to bottom when adding message while linked to an old action. This PR fix it.
Fixed Issues
$ #46290
PROPOSAL: #46290 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mwbe.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4