kidroca
commented
3 years ago Proposition
Remove componentDidUpdate from ReportActionCompose
Remove the life cycle method or at least the part causing the most side effects - the call to setIsFocused. This is the most damaging effect and gives the component too much control.
To implement this behavior we shouldn't make the view decide - it'll need too much external knowledge to guess correctly - "am I currently focused. Did a modal just close? ". Instead we should tell the view when to focus (from outside). There are different approaches we can discuss.
On native layers we like to have the Text Input not focused so the user can read new chats without they keyboard in the way of the view
This is a comment inside TextInputFocusable
This component has env specific implementations for native and web, we can do something there to ensure auto focus is not triggered on mobile, that it only works when you tap the field. Then opening reports, closing modals would autofocus on web but not on mobile native (Do consider you probably want the same behaviour for mobile web too). An extension if programmatically controlling focus is needed can be to implement our own focus interface and have different implementations per env e.g. field.focus(reason) when reason is “autofocus” do nothing on mobile, otherwise focus. Alternatively using Platform code to decide whether to autofocus might be something to consider
Overall I think the Report view/screen should tell the input to focus when navigation returns or goes to a Report - on mobile or mobile web this need to be skipped as it pops the keyboard while on web/desktop it’s convenient. The most simplest and easy to understand way is using Platform specific code in the handle as whoever is reading the code can see this behaviour is platform specific, and code doesn’t need to be added/changed anywhere else
Preventing accidental focus on the input
I need to study the navigation a bit more to see how can we prevent the text input from accidentally stealing focus when the chat view is not active (e.g. keyboard tab navigation). I think it might already be handled by react navigation stacking, and the issue is just with the drawer I've mentioned this but this check:
ReportActionSelect.render
const inputDisable = this.props.isSmallScreenWidth && Navigation.isDrawerOpen();Render would be called when props change, if they don't change and the drawer opens this won't be evaluated
ReportView: Add loading state when transitioning to a new chat/report
My original idea was: Sub views of ReportView can be unmounted and replaced by placeholders or stubs when they aren't ready to be presented
Getting to know the code I see the view responsible for loading content is actually ReportActionsView. I need to study it more, but it seems that an initial update might be to add some loading state handling there in order to smooth the transition form report to report e.g:
ReportActionsView.render
// Comments have not loaded at all yet do nothing
if (!_.size(this.props.reportActions)) {
return null;
}reportActions comes directly from Onyx, I guess with a slight delay. while reportID is passed by the parent. This is why you briefly see the old view
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.
Even simply forcing the views inside ReportView to remount on reportID change is a bit of an improvement:
marcaaron
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
I'm opening this ticket as a follow up discussion related to https://github.com/Expensify/Expensify.cash/issues/1855#issuecomment-808135377
Problems
Side effects caused by
componentDidUpdateinReportActionComposeSide effects due to the text field inside
ReportActionComposecan steal or keep focus to itselfSwitching between reports (chats) show the previous reports briefly. Then content is swapped and some repositioning happens while more content is loading
https://user-images.githubusercontent.com/12156624/112819293-1ca07b80-908d-11eb-8164-cdd7082e3e78.mp4
Related app modules/components
The ReportActionCompose is the component used to type messages, add attachments and other chat related interactions. It is been known to cause some side effects
The ReportView Displays a chat/report by wrapping
ReportActionsView(list of chat items) andReportActionComposeThese components are part of the "Home" screen of the app. Without going into great detail - it's a screen at the root of the navigation and the rest of the screens would be stacked on top of it. It's something that would rarely re-mount (if ever)
"Home" is a DrawerNavigator rendering a
ReportScreenand aSidebarScreenThese two are always rendered together even though on smaller screens only one of them is visible at a time. The sidebar lists chats/reports and the report screen renders the selected reportExpected Result:
Prevent the input field in
ReportActionComposefrom getting focus while we're away from theReportView.We are considered away when:
ReportView. This would happen only on small screens - mobile and mobile web.Focus the compose text field when:
Quoted from https://github.com/Expensify/Expensify.cash/pull/2022#issuecomment-808410246
Note: this is web/desktop specific behaviour as on mobile and mobile web a focus will bring up the keyboard and hide a big chunk of the screen
Provide a smoother transition between reports
Version Number: Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: