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.

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: ReportScreen is blank, indefinitely loading on web/desktop #2322

Merged

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Apr 9, 2021

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
@marcaaron

Details

Addresses the regresion found for #2221
The details and the steps are the same as in that PR

Fixed Issues

Fixes #2327

Tests

On web/desktop

  1. Be logged out
  2. Log in
  3. Soon after loading should stop and you should see the chat view
  4. There should not be a loader, loading indefinitely

Also please cover the steps in #2221

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@kidroca kidroca requested a review from a team as a code owner April 9, 2021 16:39
@MelvinBot MelvinBot requested review from yuwenmemon and removed request for a team April 9, 2021 16:39
marcaaron
marcaaron previously approved these changes Apr 9, 2021
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM

@kidroca
Copy link
Contributor Author

kidroca commented Apr 9, 2021

Updated
I'm done pushing more stuff, unless you see something that needs changing here

@shawnborton
Copy link
Contributor

Hmm so wait, why even have a loader at all?

@kidroca
Copy link
Contributor Author

kidroca commented Apr 9, 2021

The issue was with the old chat being briefly visible and then a lot of heavy rendering would make the chat appear in chunks

2021-03-29.12-45-06.mp4

The idea was to end up with something like this, but it can't be all addressed in a single PR as it's complex

loader.mp4

We have come this far (this is a dev build on simulator so it's not the smoothest example, and the keyboard popping out doesn't help):

Screen.Recording.2021-04-08.at.22.19.36.mov
  • ✔️ the old chat no longer appears initially
  • ⚠️ the transition can be improved
  • ❌ chat items rendering can be improved

@shawnborton
Copy link
Contributor

Got it. Yeah, I think fading in/out might help. Then this way if the transition is able to happen fast enough (like the internet connection is fast) then maybe we don't even get a chance to see the loader.

@jboniface
Copy link

Any chance that the update this PR is addressing led to the choppy behavior we're seeing on iOS in this video?

Image.from.iOS.MP4

cc @joaniew

@kidroca
Copy link
Contributor Author

kidroca commented Apr 9, 2021

@jboniface

Any chance that the update this PR is addressing led to the choppy behavior we're seeing on iOS in this video?

It doesn't seem the video includes the changes from this PR as it would display a spinner, fading out, for at least a split second

Here's a before/after comparison

Right before my changes were merged - 09e4a86

iOS.Before.mp4

With my changes merged - db7b937

iOS.After.mp4

Some of my prototypes included Layout Animation. But we agreed they would have to be applied incrementally to keep the changes under the PR small. This is something I was experimenting 2-3 days ago and hadn't had merged any updates from master - you can notice there's no bump that seems to have crept some time in between...

With.Layout.Animations.mp4

@kidroca
Copy link
Contributor Author

kidroca commented Apr 9, 2021

@marcaaron
I had an alternative loading handling, it makes the component a bit more complex, but will hide the loading as soons as there is data from Onyx, which would be instantly for web/desktop. You can take a look here:
kidroca@ae376b4

@kidroca
Copy link
Contributor Author

kidroca commented Apr 13, 2021

This indefinitely loading issue should be fixed by #2346 . unless we want to apply some small changes to the full screen loader - color, opacity, remove it, we can close this PR.

If this will be kept open I'd like to reset it to master and just apply the loader changes

@marcaaron
Copy link
Contributor

@kidroca I think based on this conversation here we are expecting to see some color updates with just a softer background.

@marcaaron
Copy link
Contributor

Which is pretty much what we've got and just need to fix conflicts and remove the unneeded code fixed in the other PR (now merged).

@kidroca kidroca force-pushed the kidroca/report-view-initial-report-fix branch from 2bdc554 to 217bf21 Compare April 14, 2021 06:58
@kidroca
Copy link
Contributor Author

kidroca commented Apr 14, 2021

Updated.
The PR now contains only a bg color change to the fullscreen loader

@kidroca kidroca requested a review from marcaaron April 14, 2021 07:05
@marcaaron marcaaron merged commit 63edf21 into Expensify:master Apr 14, 2021
@OSBotify
Copy link
Contributor

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

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.

Web - Chat- Loading spinner displayed on login in conversation screen
5 participants