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

fix#29319 - ios dismiss modal #31500

Conversation

intergalacticspacehighway
Copy link
Contributor

@intergalacticspacehighway intergalacticspacehighway commented May 9, 2021

This PR aims to resolve iOS can't dismiss Modal on swipe gesture.
#29319

Summary

When modal presentationStyle is pageSheet, iOS allows to dismiss the modal using swipe gesture. This PR adds support for that feature

Changelog

[iOS] [Added] - Support for onRequestClose for iOS Modal component.

Test Plan

  • If onRequestClose updates the visibility state, modal will be closed.
<Modal
    visible={visible}
    animationType="slide"
    presentationStyle="pageSheet"
    onRequestClose={dismiss}>
</Modal>
Screen.Recording.2021-05-10.at.4.56.54.AM.mov

Notes

  • In this PR, only support for partial drag is added. i.e. user can't drag the modal up and down completely. I added full user dragging but reverted in this commit to support controllable onRequestClose. If someone has any suggestion to have full draggable support + controllable onRequestClose, please let me know.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 9, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: ae4946f

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,986,729 +0
android hermes armeabi-v7a 8,493,977 +0
android hermes x86 9,439,010 +0
android hermes x86_64 9,380,773 +0
android jsc arm64-v8a 10,605,129 +0
android jsc armeabi-v7a 10,105,423 +0
android jsc x86 10,624,145 +0
android jsc x86_64 11,207,819 +0

Base commit: ae4946f

@oarsoy
Copy link

oarsoy commented Aug 1, 2021

When this will be merged? It doesn't make any sense.

@feedthejim feedthejim self-requested a review August 2, 2021 09:11
@facebook-github-bot
Copy link
Contributor

@sammy-SC has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sammy-SC
Copy link
Contributor

sammy-SC commented Aug 2, 2021

@intergalacticspacehighway awesome! Thank you for working on this.

We might need to change documentation for Modal onRequestClose

@zhahaoyu
Copy link

Hi @sammy-SC @intergalacticspacehighway How can we merge this PR? this is open for a couple months now.

@intergalacticspacehighway
Copy link
Contributor Author

@sammy-SC Updated the docs for the same. Let me know if any changes are needed! :)

@zhahaoyu
Copy link

@feedthejim Could you review this and merge?

@henrymoulton
Copy link
Contributor

@sammy-SC thank you for previously taking a look at this. It looks like @intergalacticspacehighway updated the docs, is there anything else outstanding?

@facebook-github-bot
Copy link
Contributor

@sammy-SC merged this pull request in c29ec46.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants