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

Switching between reports (chats) show the previous reports briefly. #2154

Closed
kidroca opened this issue Mar 30, 2021 · 17 comments · Fixed by #2221
Closed

Switching between reports (chats) show the previous reports briefly. #2154

kidroca opened this issue Mar 30, 2021 · 17 comments · Fixed by #2221
Assignees
Labels
External Added to denote the issue can be worked on by a contributor

Comments

@kidroca
Copy link
Contributor

kidroca commented Mar 30, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

Switching chats should transition smoothly between existing chats

Actual Result:

When you open a different chat you can briefly see the old chat opened. Then new content abruptly replaces it

Action Performed:

  1. Have at least 2 chats/conversations
  • they should both have some text and attachments
  1. Open the first conversation
  2. Go back / expand the sidebar
  3. Select the second conversation
  4. Observe old conversation is briefly visible and the app having a hard time to keep up with rendering

Workaround:

Not needed. The app still functions

Platform:

Where is this issue occurring?

[x] Web
[x] iOS
[x] Android
[x] Desktop App
[x] Mobile Web

The issue occurs most notably on mobile platforms

Version Number: expensify.cash@1.0.6-3
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

2021-03-29.12-45-06.mp4
@kidroca
Copy link
Contributor Author

kidroca commented Mar 30, 2021

I would like to work on this:

Proposal

Add loading state in ReportView when reportID changes

Getting to know the code I see the view responsible for loading content is actually ReportActionsView.

Instead of null we can make it render a full screen loader while enough content is prepared to be displayed

ReportActionsView.render

// Comments have not loaded at all yet do nothing
if (!_.size(this.props.reportActions)) {
    return null;
}

I can add a loading state to the component or use existing functionality from Onyx if such is available. And then update the render method to render a full screen loader like this one:

loader.mp4
  • This is the Medium app loading a sub view

Of course if there's an existing component I can use it instead.
I'll need some designs if this is approved


For future consideration perhaps some of the loading/fetching can be moved to ReportView - for example the bare minimum information needed. When reportID change - start loading, mount some loading state component/s and then remount report components passing their initial state - the loaded data, which should help reduce some of the current logic inside life cycle hooks like componendDidUpdate e.g. logic that currently needs to be performed for both componentDidMount and componentDidUpdate would be possible to happen only in componentDidMount

@Julesssss
Copy link
Contributor

Thanks for this report.

I think a better fix would be to separate out the ChatList and Chat pages (currently we treat the ChatList as an overlapping menu for the Chat page).

@kidroca
Copy link
Contributor Author

kidroca commented Mar 31, 2021

I think a better fix would be to separate out the ChatList and Chat pages (currently we treat the ChatList as an overlapping menu for the Chat page).

As long as the loading is handled inside ReportActionsView - no matter how Chat and ChatList are implemented you'd have this lack of a smooth transition

I don't think ChatList and Chat can be easily separated. I thought the ChatList can move to a separate route in the nav stack, but on web/desktop these two components need to be rendered together. The easiest and common solution to achieve this is exactly as current configuration with a sidebar/drawer

Even if they are separate they would still be both mounted as the ChartList would naturally come lower in the stack than Chat - you first need to select a chat from the list then move to that chat, which means that the list becomes the previous view in the stack and stays mounted

@marcaaron
Copy link
Contributor

I think a better fix would be to separate out the ChatList and Chat pages (currently we treat the ChatList as an overlapping menu for the Chat page).

For what it's worth, I have tried many configurations and not yet found a clear way to achieve this. While there are some existing problems with the drawer navigator pattern I think it might be best for the design we have. Open to suggestions for how to get something like a pure stack navigator working with the desktop layout, but it may come with its own set of challenges to be sure.

@kidroca So we will wait for the reportID to change then initiate a loader until a small set of actions is loaded? Sounds good. We may also need to perform similar checks in the header as it's a separate component.

@kidroca
Copy link
Contributor Author

kidroca commented Mar 31, 2021

My initial idea was to address this progressively without changing much of the code.
A radical approach can be to move loading here: https://github.com/Expensify/Expensify.cash/blob/master/src/pages/home/ReportScreen.js

And render a "loading header" and a "loading report" components


A solution with less changes would be to render an abs positioned fullscreen loader so that it covers the header as well. (It can still show some sort of a loading header and a main indicator in the middle like in the video example above).

The implementation also relies on the design of the loader though...

@marcaaron marcaaron added the External Added to denote the issue can be worked on by a contributor label Mar 31, 2021
@MelvinBot
Copy link

Triggered auto assignment to @marklouisdeshaun (External), see https://stackoverflow.com/c/expensify/questions/7703 for more details.

@marcaaron
Copy link
Contributor

@marklouisdeshaun this has been triaged already so we just need an Upwork job created

@marcaaron
Copy link
Contributor

render an abs positioned fullscreen loader

Oh that's interesting. Not really how I imagined it would work though. Probably we should show loaders for any components that are "reloading" in response to the change in props. Implementing this in the ReportScreen means some logic will need to be cleaned up.

https://github.com/Expensify/Expensify.cash/blob/60e48069709edbbfdd47e65a14a8e4b6bed89242/src/pages/home/report/ReportActionsView.js#L183-L195

Let us know if you want to address this now. I think it's OK to start with a minimal solution. It can always be moved to a mounting lifecycle later if the design changes.

@kidroca
Copy link
Contributor Author

kidroca commented Mar 31, 2021

Yes, I would like to work on this. I'm finishing my other ticket and I have available time

The "big" solution might not be so big - I'll get a better estimate when I start working on this. If it turns out it doesn't need much changes - it's easy to review and test I can address it here

@marklouisdeshaun
Copy link
Contributor

Upwork job posting here. @kidroca can you please apply to that job on Upwork?

@kidroca
Copy link
Contributor Author

kidroca commented Mar 31, 2021

@marklouisdeshaun Just did: https://www.upwork.com/ab/proposals/1377366625381879809

@kidroca
Copy link
Contributor Author

kidroca commented Apr 20, 2021

@marklouisdeshaun
Hey, sorry to bring this up here but I didn't receive a bonus for finding this issue and documenting it in a ticket as per proposing-a-job-that-expensify-hasnt-posted

For example my other ticket did receive a bonus #2109

@Julesssss
Copy link
Contributor

Apologies, we're still working out how to keep track of bonus payments. Please continue to point out anything that is missed!

Are you currently working on another UpWork issue?

@kidroca
Copy link
Contributor Author

kidroca commented Apr 20, 2021

Not currently (but hopefully soon). I have one more Expensify job in Upwork that's pending payment - waiting for the regression time to pass

@Julesssss
Copy link
Contributor

Julesssss commented Apr 20, 2021

Great. Can you let me know which issue is pending payment please, and I'll make a note on it that we should payout the missing bonus as part of that UpWork task.

@kidroca
Copy link
Contributor Author

kidroca commented Apr 20, 2021

Thank you very much! It's this one #2325

@Julesssss
Copy link
Contributor

FYI, I've left a note on our internal issue here that we should pay an additional bonus when completing this outstanding Upwork Contract

So this ^ should be taken care of soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants