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

Close Keyboard on iOS/Android using Swipeable View #2074

Merged

Conversation

barun1997
Copy link
Contributor

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>

Details

I created SwipeableView to wrap ReportActionCompose Component.

Not sure if I should make SwipeableView cross-platform and separate ReportView to have SwipeableView wrap ReportActionCompose in iOS instead. Please let me know what you suggest!

Fixed Issues

Fixes GH_LINK #1870

Tests

  1. Click on a chat with someone
  2. Type something (toggle software keyboard on if you're on iOS Simulator)
  3. Swipe down on the message window
  4. Verify swiping down closes the keyboard

Tested On

  • iOS

Screenshots

IOS

Screen.Recording.2021-03-25.at.09.46.31.mov

@barun1997 barun1997 requested a review from a team as a code owner March 25, 2021 04:12
@botify botify requested review from timszot and removed request for a team March 25, 2021 04:12
@barun1997
Copy link
Contributor Author

barun1997 commented Mar 25, 2021

@marcaaron -- My initial proposal was to use onPanResponderMove and close the keyboard using it. But it led to too many callbacks, and on second thought, I thought calling the onSwipeDown callback on onPanResponderRelease -- when the gesture has completed, was a better solution. Please let me know what you think!

src/components/SwipeableView/index.ios.js Outdated Show resolved Hide resolved
src/components/SwipeableView/index.ios.js Outdated Show resolved Hide resolved
src/components/SwipeableView/index.ios.js Outdated Show resolved Hide resolved
src/components/SwipeableView/index.ios.js Outdated Show resolved Hide resolved
src/components/SwipeableView/index.ios.js Outdated Show resolved Hide resolved
src/components/SwipeableView/index.js Outdated Show resolved Hide resolved
@barun1997 barun1997 changed the title Add SwipeableView to wrap ReportActionCompose in iOS Close Keyboard on iOS/Android using Swipeable View Mar 26, 2021
@barun1997 barun1997 requested a review from marcaaron March 26, 2021 20:47
@barun1997
Copy link
Contributor Author

@marcaaron I have made SwipeableView available in Android as well and addressed other issues. Please let me know if you have any suggestions!

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks nice and simple, but noticing that swipes are not registered on the text input field itself. I think makes this less useful than we were hoping since it's kind of hard to swipe on the smaller areas around the text input to get the keyboard to dismiss?

Example:

2021-03-26_10-45-00.mp4

@barun1997
Copy link
Contributor Author

barun1997 commented Mar 26, 2021

@marcaaron This should fix it! The text input field seems to be receiving touch gestures now.
https://user-images.githubusercontent.com/19339044/112693537-e6a2a200-8ea8-11eb-8aea-0923d373e4fd.mov

ios/Podfile.lock Outdated Show resolved Hide resolved
@marcaaron
Copy link
Contributor

Tests great and working well. Once the Podfile.lock is resolved I will merge.

@barun1997
Copy link
Contributor Author

barun1997 commented Mar 29, 2021

Hi @marcaaron, I reverted the changes in Podfile.lock with a new commit.

@marcaaron
Copy link
Contributor

Hi @marcaaron, I reverted the changes in Podfile.lock with a new commit.

Sorry, it's still there. Maybe you did not push the change?

@barun1997
Copy link
Contributor Author

Hi @marcaaron, I reverted the changes in Podfile.lock with a new commit.

Sorry, it's still there. Maybe you did not push the change?

Does 49f729c not resolve it?

@marcaaron
Copy link
Contributor

Sorry, no, I think because you removed a new line from this file. It should not show as one of the changes files.

@barun1997
Copy link
Contributor Author

@marcaaron Ah you're right :) I've updated it. Thank you for your patience! 💯

@marcaaron marcaaron merged commit 8eaa463 into Expensify:master Mar 29, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants