Skip to content
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.

Sign up for GitHub

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

Fix chat doesn't scroll to bottom when adding message while linked to an old action #46724

Merged
merged 3 commits into from
Aug 16, 2024
Merged

Fix chat doesn't scroll to bottom when adding message while linked to an old action #46724

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fc45d1
fix chat doesn't scroll to bottom when adding message while linked to…
bernhardoj Aug 2, 2024
2fc9ebc
add missing deps
bernhardoj Aug 2, 2024
1c2de3e
Merge branch 'main' into fix/46290-report-doesnt-scroll-to-bottom-whe…
bernhardoj Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/pages/home/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,12 +364,16 @@ function ReportActionsList({
(isFromCurrentUser: boolean) => {
// If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
// they are now in the list.
if (!isFromCurrentUser || !hasNewestReportActionRef.current) {
if (!isFromCurrentUser) {
return;
}
if (!hasNewestReportActionRef.current) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to follow the same logic of scrollToBottomAndMarkReportAsRead.

App/src/pages/home/report/ReportActionsList.tsx

Lines 415 to 421 in 55a29dd

const scrollToBottomAndMarkReportAsRead = () => {
if (!hasNewestReportAction) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
Report.openReport(report.reportID);
return;
}
reportScrollManager.scrollToBottom();

I guess we need to hold for #46627 then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

@bernhardoj bernhardoj Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more talking about why scrolling doesn't work on Android native when go back to the main chat and send message

Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, just reinstalled android app
And everything works
Could you please reinstall ios and android native apps and record videos for checklist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it works. Completed recordings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job
Thank you !

}
InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
},
[reportScrollManager],
[reportScrollManager, report.reportID],
);
useEffect(() => {
// Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function?
Expand Down
Loading