Added transition handling between the side drawer and chats

[kidroca]

@marcaaron

Details

Add a loading transition when chat is selected and the side drawer closes

1. Added a fullscreen loading component - just something for the time being - it needs to be revisited with proper design
2. Added withDrawerState HOC
3. Disable the comose input component when the drawer is extended - this fixes some issues with the keyboard staying open

• doesn't work great on Android but has no side effects, the keyboard can still stay open sometimes. It works on iOS and mWeb the keyboard would always close when you open the drawer

Fixed Issues

Fixes #2154 fixes #2150

Tests

1. From the home screen
2. Have a few conversation
3. Select a conversation
4. The conversation should open with a brief loading

• there should be a fullscreen loading indicator (centered)
• on physical device sometimes loading happens so fast you might not see the loader. The transition should be smooth though

5. Go back and select another conversation

• There's shouldn't be any glimpse of the previous conversation
• The transition should feel as you are opening a new page

The keyboard should be dismissed when you go back to the LHN

• when the keyboard is visible on the screen going back (opening the drawer) should dismiss it
• This should work 100% on iOS and on mWeb (mobile Safari and Chrome)
• For some reason it doesn't work always work on Android for me
• [HOLD] Going back to main list after keyboard call won't close keypad #2150 is left open due to the Android problem

QA Steps

Same as the Tests above

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Expensify.cash.-.Google.Chrome.2021-04-08.22-20-51.mp4

Mobile Web

Screen.Recording.2021-04-08.at.22.37.58.mov

Desktop

Screen.Recording.2021-04-08.at.23.14.44.mov

iOS (Simulator)

Screen.Recording.2021-04-08.at.22.19.36.mov

Android

Android.Emulator.-.Pixel_2_API_29_5554.2021-04-08.22-10-54.mp4

[kidroca]

@marcaaron This is my rough sketch of how things can work
I want to share it early on to address any concerns

This is a draft - Please don't pay attention to stuff that needs to be cleaned up. There are some lint errors and I'm yet to move styles, add documentation etc...

There are a few items I want to ask:

• I've read you don't want to use promises - is the usage here OK - I can replace it with check in componentDidUpdate, but it won't be as elegant as now
• I need to implement optimistic response - e.g. don't wait for fetchActions if the component has some data available to render immediately. I need to think about this but if you have an idea
• The fullscreen loader might actual not be needed at all. I've tried without it and the transition is still very good. I replaced it with null 😄. I can add a sample video next week
  • I think the loader would need further work but that can be moved to another PR
  • you can skip reviewing it for the moment, until I'm ready with the draft

So all in all I've still got some work to polish this but what do you say?

[kidroca]

Updated
Added videos and tested on all Platforms
Check the desktop/web transitions - maybe we should skip transition on big screens, as they currently work well.

[marcaaron]

Thanks, totally missed the addition of the key. Makes sense now.

[marcaaron]

Love the code changes here. Thanks!

The full page loader is slightly strange on web/desktop/larger screens, but I think we can address that in a follow up if anyone has major issues with it.

[laurenreidexpensify]

Assuming no regressions in next week @kidroca this will be paid on 15 April 👍🏽

[parasharrajat]

Opening the Url expensify.cash directly. shows this screen infinitely.

[marcaaron]

@kidroca I think this is because we've switched from using the currentlyViewedReportID to the route.params.reportID. I missed that change in the PR. So, if we want to do it this way then we'll need to implement the solution discussed here:

https://expensify.slack.com/archives/C01GTK53T8Q/p1617908395456000

We can either:

1. Implement this fix now and not create a new issue for it
2. Revert this change by going back to using the currentlyViewedReportID temporarily and then create a new issue to switch things to route params.

[kidroca]

The proper fix is to address the navigation it's related to other issues like the URL changing on mWeb, but I can't do it in the scope of the current ticket. If you want me to work on that I'll need a new ticket.

I can revert to using currentlyViewedReportID like it did before, though this will degrade the experience on Android. Will post a video

[kidroca]

Here's how it would look like on Android when I revert to currentlyViewedReportID - the prop only updates after a delay.

Android.Report.Switch.mp4

This is an issue with slow AsyncStorage access speed. I don't know how exactly ONYX works, but it might be possible to add a small proxy layer that persists data but keeps in in memory as well, so when it is accessed it can come instantly and don't have to be read from the file system first.
I think the iOS implementation of AsyncStorage probably does something like this as it works instantly there, so this might be a feature that can be implemented for AsyncStorage - configurable in memory proxy for fast access

[marcaaron]

I leave that up to you @kidroca. It's a regression from what we had before so we must do something for now :) I agree that switching the reportID source was probably not in scope so the fix for it doesn't need to be either.

p.s. I think maybe the video shared is to show some other behavior than what you are describing.

[kidroca]

p.s. I think maybe the video shared is to show some other behavior than what you are describing.

Yep, sorry, updated the comment

Btw I thought of another patch fix that would better address this at the moment, I'll open a new PR. Should I copy the current PR description there?

[marcaaron]

Should I copy the current PR description there?

Ah, no need, linking back to this PR should be enough.

[isagoico]

We found this issue that looks related to this PR #2327. Not completely sure since the issue is reproducible in production.

[roryabraham]

More than a year ago that this was added now, but wondering if there was any particular reason this side-effect was put in shouldComponentUpdate instead of componentDidUpdate?

[roryabraham]

Is it because you were hoping to avoid having to evaluate _.isEqual(nextProps.reportAction, this.props.reportActions) in both shouldComponentUpdate and componentDidUpdate`?

[marcaaron]

Not exactly... componentDidUpdate() happens after the render() so (as things are now) if we sorted the actions there then we'd also need to trigger an re-render somehow.

Using shouldComponentUpdate() with a class property is a way to re-sort actions before the render() method is called.

I guess a more current approach could be to recalculate the options in the render with a dependency on props.reportActions, but my hook fu is pretty weak tbh 😂

[kidroca]

More than a year ago that this was added now, but wondering if there was any particular reason this side-effect was put in shouldComponentUpdate instead of componentDidUpdate?

Marc already answered, but yes, to capture sortedActions before rendering and without triggering a re-render

I guess a more current approach could be to recalculate the options in the render with a dependency on props.reportActions

If your talking about hooks it would be as simple as

const sortedActions = useMemo(() => util.sortActions(props.reportActions), [props.reportActions])

But then the component would need to be converted from a class to a function based component (to use hooks)

For a class component the only way to sort actions, (only) when the prop changed, is to use shouldComponentUpdate

---

Another alternative is to subscribe the view to reportId actions, and sort them in the connection configuration, so the component always receives sorted actions (they'd still be sorted only if they change)

[marcaaron]

Yep, that's sort of what I had in mind. But, I think _.isEqual() is doing an optimized deep comparison whereas useMemo() would only run if the reportActions failed a simple object comparison? (so even better)

Another alternative is to subscribe the view to reportId actions, and sort them in the connection configuration, so the component always receives sorted actions (they'd still be sorted only if they change)

That's pretty interesting. If we are doing a keyChanged() then we know the subscriber is getting new data and it's an opportunity to run some logic without having to do the deep compare.

That said, I think we are maybe always creating new objects when merging or setting changes so when calling keysChanged() the simple compare in useMemo() might be all that we'd need. When keyChanged is called the object reference for something like props.reportActions would be pointing to a new object.

[marcaaron]

This conversation here is slightly related -> Expensify/react-native-onyx#185

[roryabraham]

Another alternative is to subscribe the view to reportId actions, and sort them in the connection configuration, so the component always receives sorted actions (they'd still be sorted only if they change)

This is pretty interesting, but I think overall the hook-based approach might be simpler/more accessible to more people in the long term?

Anyways, thanks for the thorough answers here!