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

Standardize on one method for detecting keyboard state #13582

Closed
tgolen opened this issue Dec 14, 2022 · 7 comments
Closed

Standardize on one method for detecting keyboard state #13582

tgolen opened this issue Dec 14, 2022 · 7 comments
Assignees
Labels
Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@tgolen
Copy link
Contributor

tgolen commented Dec 14, 2022

Problem

There are two ways for components to detect the keyboard state:

  1. Using the HOC withKeyboardState - this works on native devices, but doesn't have support for mobile web
  2. Using VirtualKeyboard.shouldAssumeIsOpen() - this appears to work on both native devices and mobile web. Originally added in this PR.

Neither option is used consistently, leading to inconsistent behavior in the app.

Solution

  • Make sure that VirtualKeyboard works for mobile web
  • Move the functionality from VirtualKeyboard into withKeyboardState
  • Use withKeyboardState everywhere
  • Remove VirtualKeyboard

CC @jasperhuangg @parasharrajat

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e011fc436edaf3d0
  • Upwork Job ID: 1608620601075437568
  • Last Price Increase: 2022-12-30
@tgolen tgolen added Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Bug Something is broken. Auto assigns a BugZero manager. labels Dec 14, 2022
@tgolen tgolen self-assigned this Dec 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 14, 2022

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 14, 2022
@tgolen tgolen removed the Bug Something is broken. Auto assigns a BugZero manager. label Dec 14, 2022
@tgolen
Copy link
Contributor Author

tgolen commented Dec 14, 2022

Sorry @flaviadefaria, this is not a bug, just a cleanup issue, so I am removing your assignement.

@tgolen tgolen added the Improvement Item broken or needs improvement. label Dec 14, 2022
@tgolen
Copy link
Contributor Author

tgolen commented Dec 14, 2022

All right, here is what I found out. Virtual keyboard exports two methods:

  1. isOpen() - Is always the same as props.isShown on all platforms and it's only used in one place here. That can easily be replaced with props.isShown
  2. shouldAssumeIsOpen() - Is always the same as props.isShown for native devices. It always returns true for mobile web platforms. It's used in 4 places and is almost always used as a proxy for "is this mobile web?" which is also the same as isSmallScreenWidth (usually)

So, I think that all of this can be refactored to just use the two HOCs we currently have for withKeyboardState and withDimensions and we can remove the VirtualKeyboard lib entirely.

@tgolen
Copy link
Contributor Author

tgolen commented Dec 28, 2022

Hm, I'm not sure why GH didn't link the PR with this issue #13629 but this was deployed to production, so I'm going to close this.

@tgolen tgolen closed this as completed Dec 28, 2022
@mallenexpensify mallenexpensify self-assigned this Dec 30, 2022
@mallenexpensify mallenexpensify added Internal Requires API changes or must be handled by Expensify staff and removed Internal Requires API changes or must be handled by Expensify staff labels Dec 30, 2022
@mallenexpensify
Copy link
Contributor

@thesahindia can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01e011fc436edaf3d0

@Expensify Expensify unlocked this conversation Dec 30, 2022
@thesahindia
Copy link
Member

Accepted, thanks!

@mallenexpensify
Copy link
Contributor

Paid @thesahindia , closing cuz this shouldn't need regression test steps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

4 participants