Skip to content

Fix : Chat moves up a bit when new message is received#7271

Merged
mountiny merged 1 commit into
Expensify:mainfrom
ahmdshrif:fix/chat-moves-on-new-message
Jan 17, 2022
Merged

Fix : Chat moves up a bit when new message is received#7271
mountiny merged 1 commit into
Expensify:mainfrom
ahmdshrif:fix/chat-moves-on-new-message

Conversation

@ahmdshrif

@ahmdshrif ahmdshrif commented Jan 16, 2022

Copy link
Copy Markdown
Contributor

Details

  • fix Chat moves up a bit when new message is received for ios

Fixed Issues

$ #7145

Tests

  • Verify that no errors appear in the JS console

QA Steps

  1. Open a room and scroll to earlier message (fastly)
  2. Stay on specific message (to read it or something)
  3. Send new message to that room
  4. Verify that chat view is not moving around when new message is received
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-chat-move-up.mov

Mobile Web

Screen_Recording_20220116-221221_Samsung.Internet.mp4

Desktop

desktop.mov

iOS

Screen.Recording.2022-01-16.at.6.04.52.PM.mov

Android

@ahmdshrif ahmdshrif requested a review from a team as a code owner January 16, 2022 19:18
@MelvinBot MelvinBot requested review from mountiny and rushatgabhane and removed request for a team January 16, 2022 19:19

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! Good find here @ahmdshrif:)

I will wait for Rushat to test this and approve it before going ahead.

@rushatgabhane

Copy link
Copy Markdown
Member

I'm testing for regressions. Meanwhile, @ahmdshrif please add screenshots for all platforms. Thanks a lot! 😃

@ahmdshrif

Copy link
Copy Markdown
Contributor Author

@rushatgabhane I have already included a video for all platforms except android since it still has the issue.

rushatgabhane
rushatgabhane previously approved these changes Jan 16, 2022

@rushatgabhane rushatgabhane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mountiny Looks good to me too! Couldn't find any regressions.

mountiny
mountiny previously approved these changes Jan 16, 2022

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Thank you very much @rushatgabhane and @ahmdshrif!

:shipit:

@mountiny

Copy link
Copy Markdown
Contributor

Yikes, I cant merge this as your commit is not signed it seems @ahmdshrif :/

Would you be able to follow these instructions and I guess force push to this branch to sign the commit. https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#begin-coding-your-solution-in-a-pull-request

Thank you!

@ahmdshrif ahmdshrif dismissed stale reviews from mountiny and rushatgabhane via d197f49 January 16, 2022 23:51
@ahmdshrif ahmdshrif force-pushed the fix/chat-moves-on-new-message branch from 27ac6c9 to d197f49 Compare January 16, 2022 23:51
@ahmdshrif

Copy link
Copy Markdown
Contributor Author

@mountiny ok, I signed it and delete the not-signed commit.

mountiny
mountiny previously approved these changes Jan 17, 2022

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @ahmdshrif!

rushatgabhane
rushatgabhane previously approved these changes Jan 17, 2022
@ahmdshrif ahmdshrif closed this Jan 17, 2022
@ahmdshrif ahmdshrif force-pushed the fix/chat-moves-on-new-message branch from d197f49 to 74cf52f Compare January 17, 2022 02:12
@ahmdshrif ahmdshrif reopened this Jan 17, 2022
@ahmdshrif ahmdshrif dismissed stale reviews from rushatgabhane and mountiny via e5b98a9 January 17, 2022 02:13
@ahmdshrif

Copy link
Copy Markdown
Contributor Author

sorry for that again. but my commit was signed but was not verified so I again push force verified sign commit and that dismiss your reviews.
@mountiny @rushatgabhane

@ahmdshrif ahmdshrif requested a review from mountiny January 17, 2022 03:56
@mountiny

Copy link
Copy Markdown
Contributor

Thank you!

@mountiny

Copy link
Copy Markdown
Contributor

Test passed so going ahead and merging.

@mountiny mountiny merged commit 2b7f6a7 into Expensify:main Jan 17, 2022
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @mountiny in version: 1.1.30-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kbecciv

kbecciv commented Jan 19, 2022

Copy link
Copy Markdown

@ahmdshrif @rushatgabhane @r8 The issue is reproduced during Regression.

moves.1901.mp4

@ahmdshrif

Copy link
Copy Markdown
Contributor Author

@kbecciv think the issue is defined on android and ios only.

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.27-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

RPReplay_Final1641962371.MP4
Expensify/Expensify Issue URL: Issue reported by: @K4tsuki Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1641188993370400

@rushatgabhane

This comment has been minimized.

@rushatgabhane

rushatgabhane commented Jan 19, 2022

Copy link
Copy Markdown
Member

Alright, this issue is happening on Safari only (both web and mWeb)

It's reproducible on main too for Safari.

@ahmdshrif

ahmdshrif commented Jan 19, 2022

Copy link
Copy Markdown
Contributor Author

Yes i test it on mobile (not safari) work

Screen_Recording_20220119-194225_Samsung.Internet.mp4

@rushatgabhane

rushatgabhane commented Jan 19, 2022

Copy link
Copy Markdown
Member

@ahmdshrif any ideas how to fix this? (Safari only issue)

@mountiny

Copy link
Copy Markdown
Contributor

Yeah, we have missed that case to test, maybe we should explicitly state it in the PR checklist too. Would you be able to look into the safari issue @ahmdshrif, please?

@ahmdshrif

Copy link
Copy Markdown
Contributor Author

@mountiny @rushatgabhane
I try to debug this but do not have a solution for now. it looks like the flatlist for web use something not supported on safari.

I test it with and without my code. it's happened on safari in both cases.

@mountiny

Copy link
Copy Markdown
Contributor

Alright, that is a shame safari is causing such problems for us. I guess we will need to follow up with this later on if there is not a clear solution for this.

@mountiny

Copy link
Copy Markdown
Contributor

Ok, I have created a follow-up issue, since I would say this has not been clearly stated before and we should not hold the payment of the linked issue on this regression. This is an edge case that will require additional treatment. #7334

@rushatgabhane

Copy link
Copy Markdown
Member

Thanks!

@ahmdshrif

Copy link
Copy Markdown
Contributor Author

thanks, mountiny rushatgabhane

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.31-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants