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

[$250] react-native-keyboard-controller migration plan #37203

Open
kirillzyusko opened this issue Feb 26, 2024 · 51 comments
Open

[$250] react-native-keyboard-controller migration plan #37203

kirillzyusko opened this issue Feb 26, 2024 · 51 comments
Assignees
Labels
Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@kirillzyusko
Copy link
Contributor

kirillzyusko commented Feb 26, 2024

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 (done in feat: edge-to-edge mode on Android #52392):
    • 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):

Additional resources

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01360efa253e7a2c60
  • Upwork Job ID: 1762155306344951808
  • Last Price Increase: 2024-04-23
Issue OwnerCurrent Issue Owner: @kirillzyusko
@roryabraham roryabraham added Weekly KSv2 Improvement Item broken or needs improvement. Bug Something is broken. Auto assigns a BugZero manager. labels Feb 26, 2024
Copy link

melvin-bot bot commented Feb 26, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 26, 2024
@roryabraham
Copy link
Contributor

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
Copy link
Contributor

roryabraham commented Feb 26, 2024

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.

@roryabraham roryabraham added the Internal Requires API changes or must be handled by Expensify staff label Feb 26, 2024
Copy link

melvin-bot bot commented Feb 26, 2024

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

Copy link

melvin-bot bot commented Feb 26, 2024

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

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

moving this to weekly

@shubham1206agra
Copy link
Contributor

@roryabraham I was already assigned #16356.

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

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

@shubham1206agra
Copy link
Contributor

@roryabraham Can you assign me here also?

@cubuspl42
Copy link
Contributor

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

@shubham1206agra
Copy link
Contributor

@cubuspl42 Please have a look at the #16356.

@cubuspl42
Copy link
Contributor

@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
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

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

correct. 2 C+ in this case

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@perunt
Copy link
Contributor

perunt commented Sep 23, 2024

It's in the review phase. Once testers confirm there's no jittering on their devices, we'll be good to go.
I asked about such testing because I was only able to test it on a few devices. On my oldest, least reliable device (an iPhone 7 with significant throttling and overheating issues), it shows jittering. However, the rest of the devices, such as the iPhone XR and 15, work well. So, the jittering might not be representative of typical user experiences

@roryabraham
Copy link
Contributor

Need someone to hand this off to. @luacmartins not sure who's best, but going to reassign to you. Maybe you can handle or pass off to another Expensify engineer

@luacmartins
Copy link
Contributor

@perunt are we still waiting for a review on #42143?

@melvin-bot melvin-bot bot removed the Overdue label Oct 15, 2024
@kirillzyusko
Copy link
Contributor Author

@luacmartins yes, review + we need to fix issues that were found 👀

@melvin-bot melvin-bot bot added the Overdue label Oct 29, 2024
@luacmartins
Copy link
Contributor

Still working on the PR.

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 25, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

This issue has not been updated in over 15 days. @JmillsExpensify, @rojiphil, @luacmartins, @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!

@kirillzyusko
Copy link
Contributor Author

@MelvinBot we are actively working on this issue. For example today #52392 was merged (where we started to use keyboard-tracking functionality on Android).

So slowly but decently we are moving 💪

@luacmartins
Copy link
Contributor

Working on v2 of the PR since the first one got reverted

@JmillsExpensify
Copy link

Still working on the PRs

@JmillsExpensify
Copy link

Same same

@JmillsExpensify
Copy link

In progress

@luacmartins
Copy link
Contributor

WIP

@roryabraham
Copy link
Contributor

Resuming my stewardship of this issue. Seems like it overlaps substantially with @hannojg's proposal here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
Status: No status
Development

No branches or pull requests

9 participants