[$250] `react-native-keyboard-controller` migration plan

kirillzyusko commented 4 months ago

Problem statement

In Expensify app keyboard handling is not consistent across iOS/Android platforms:

on Android we mostly rely on default keyboard avoiding mechanism adjustResize (it works great, but lacks of synchronous animations - which means the screen will be instantly resized, and there will be no animation at all. For chat-like apps this is quite important to have for good UX)
since Android handles it automatically - it's very hard to add any customization to keyboard handling (for example to preserve elements position which are obscured by keyboard)
on iOS we are using JS code mixture to handle keyboard avoidance (KeyboardAvoidingView, subscriptions to keyboard events) since iOS keyboard handling is done in JS code, on Android we use native keyboard avoidance provided by OS and we are writing cross-platform code -> we have a lot of Platform dependent code (like enabling KeyboardAvoidingView only on iOS), which eventually leads to UI inconsistency
react-native offers a limited API for keyboard handling (especially on Android, where we can detect only Did events)
react-native API is far behind from modern ways of keyboard handling

With react-native-keyboard-controller:

we are getting cross-platform behavior:
- edge-to-edge mode -> identical work with insets on both platforms
- disabled default keyboard handling - all keyboard handling is delegated to developer providing basics primitives
consistent API for working with the keyboard (all events are available on both platforms, including Will events which were not available prior to RN API)
interactive keyboard support - an ability to dismiss keyboard via gesture (default RN implementation only works on iOS and has severe limitations, such as multiline TextInput can not grow)

Migration plan

I see next potential way for improving keyboard handling (aka migration plan):

start to use it on a chat screen on iOS only (https://github.com/Expensify/App/pull/42143)
start to use this library on chat screen on Android (this library also will give a smooth transition on chat screen when keyboard appears/disappears, so UI will look better & more pleasant for end user)
search screen:
- right now "footer" (invite a friend, get 250$) has an instant transition and doesn't follow keyboard movement - with this library transition will be smooth (KeyboardStickyFooter component)
See it in action
- on Android when keyboard appears -> at the bottom of the screen we have a different color (under keyboard) - that's how default keyboard avoidance works in Android - when we'll have a frame-level control we can fix it as well and we'll not see any blinks there
See it in action
on Chat screen we can add "interactive keyboard dismissal" (when keyboard can be closed via gesture)
@roryabraham wanted to replace useKeyboardContext with this new library
In Profile->Address screen: auto scroll is waiting for keyboardDidShow event and we have kind of two-fold animation -> potentially can be fixed by KeyboardAwareScrollView with fully synchronized animations

Additional resources

Slack discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1707467001447909
GH issue of library review: https://github.com/Expensify/App/issues/35270
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01360efa253e7a2c60
- Upwork Job ID: 1762155306344951808
- Last Price Increase: 2024-04-23

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

roryabraham commented 4 months ago

this is kind of halfway between a new feature and a bug, but I'm going to treat it as a bug for the sake of accounting (no CAP SW project it fits with)

roryabraham commented 4 months ago

Marking this as Internal even though it will be almost 100% external. Doing so because I don't want to create an external Upwork job, but I do want a C+ to help review all the PR(s) for this. I think it will be a series of PRs, so I think we should compensate for the review of each.

melvin-bot[bot] commented 4 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01360efa253e7a2c60

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @cubuspl42 (Internal)

roryabraham commented 4 months ago

welcome @cubuspl42! I encourage you to familiarize yourself with the context on this issue and decide whether you want to take it on or pass it off. Excited to have you help if you're interested!

roryabraham commented 4 months ago

moving this to weekly

shubham1206agra commented 4 months ago

@roryabraham I was already assigned https://github.com/Expensify/App/pull/16356.

roryabraham commented 4 months ago

Fair enough. This is a broader migration than just that issue/PR though, basically we'll be refactoring keyboard handling across the whole app.

roryabraham commented 4 months ago

probably wouldn't hurt to get more than 1 C+ involved here either.

shubham1206agra commented 4 months ago

@roryabraham Can you assign me here also?

cubuspl42 commented 4 months ago

Sounds interesting; I can get on board. This will be implemented by an expert agency, and C+ reviewed, right?

shubham1206agra commented 4 months ago

@cubuspl42 Please have a look at the https://github.com/Expensify/App/pull/16356.

cubuspl42 commented 4 months ago

@shubham1206agra I took a look. It's a PR with a 1-year-long post history. Let me know what exact aspect of that PR you'dl like me to focus on!

shubham1206agra commented 4 months ago

Take a look at the top description and go through code once.

roryabraham commented 4 months ago

This will be implemented by an expert agency, and C+ reviewed, right?

correct. 2 C+ in this case

roryabraham commented 3 months ago

@kirillzyusko looks like we need to resolve conflicts in https://github.com/Expensify/App/pull/16356, then @shubham1206agra and @cubuspl42 can begin review whenever they're ready

melvin-bot[bot] commented 3 months ago

@JmillsExpensify @cubuspl42 @kirillzyusko @roryabraham @shubham1206agra this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

roryabraham commented 3 months ago

Also, this is not a bug

kirillzyusko commented 3 months ago

looks like we need to resolve conflicts in https://github.com/Expensify/App/pull/16356, then @shubham1206agra and @cubuspl42 can begin review whenever they're ready

@roryabraham done. I'll merge main branch every day to keep the Pr without conflicts 👍

JmillsExpensify commented 3 months ago

Still working on this one.

cubuspl42 commented 3 months ago

Yes, we found some bugs but they are already fixed. I'll give it another testing round today, I also bumped @shubham1206agra so they give it another pair of eyes.

JmillsExpensify commented 2 months ago

Same as above.

cubuspl42 commented 2 months ago

We have an OOO situation in SWM, the author will likely be back soon

roryabraham commented 2 months ago

agree, we'll pick this back up when @kirillzyusko is back from OOO. He works for Margelo, btw 🙂

melvin-bot[bot] commented 2 months ago

Current assignee @shubham1206agra is eligible for the Internal assigner, not assigning anyone new.

mallenexpensify commented 2 months ago

@shubham1206agra reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rojiphil (Internal)

mallenexpensify commented 2 months ago

@rojiphil reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks

roryabraham commented 2 months ago

@kirillzyusko any thoughts on when you'll be prioritizing this?

kirillzyusko commented 1 month ago

@roryabraham I think the current plan is to finish native-stack integration and after that I'll switch back to this task 👀

In a meantime @perunt is also making some progress on integrating react-native-keyboard-controller into Expensify codebase (recently I added a cursor position tracking with coordinates and it will give us an ability to avoid RN core patching and push a task with emoji autosuggestion forward).

JmillsExpensify commented 1 month ago

Are we still working on finishing native-stack ahead of this task, or is work resuming elsewhere via another issue?

kirillzyusko commented 1 month ago

@JmillsExpensify I'm working on this issue (and I updated issue description to mention that one PR link has been changed).

melvin-bot[bot] commented 2 weeks ago

This issue has not been updated in over 15 days. @JmillsExpensify, @rojiphil, @kirillzyusko, @roryabraham, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Expensify / App