react-native
react-native copied to clipboard
facebook
Allows interactive dismissal of modals on iOS
Summary:
This pull request allows the user to dismiss a modal using either pageSheet or formSheet presentation style whilst using slide animation type on iOS with an interactive gesture by explicitly opting in by setting interactiveDismissal property. Supports both old and new architectures.
Changelog:
[GENERAL] [ADDED] - Added new property interactiveDismissal to Modal props
[ANDROID] [ADDED] - Stubs a noop android method
[IOS] [ADDED] - Allows setting of modalInPresentation value in Modal UIKit files
[IOS] [ADDED] - Adds conformance to UIAdaptivePresentationControllerDelegate for the Fabric RCTModalHostViewComponentView.
[INTERNAL] [ADDED] - Adds an example to the RNTester package
Test Plan:
Clone, run the RNTester package on iOS and Android, test the interactiveDismissal prop (for which there are instructions on the modal example screen)
iOS
https://github.com/facebook/react-native/assets/121863908/3229184a-1a72-4642-8bda-5b4f2dd98179
Hi @ChrisSchofieldCheckatrade!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 16,951,561 | +507 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 20,334,992 | +147 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: 26e33a5f175e4a715d827be773efdb427ca2b52d Branch: main
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
Hi @ChrisSchofieldCheckatrade, thanks for your contribution.
As you might have seen, yesterday, some time after you opened your PR, another PR landed that changes the structure of the Modal codebase, to fix some bugs on the onDismiss event and to make this event available for Android as well.
I do think that your contribution is a valid use case for iOS. Do you think it would be possible to adapt your changes on top of that update?
Hi @ChrisSchofieldCheckatrade, thanks for your contribution. As you might have seen, yesterday, some time after you opened your PR, another PR landed that changes the structure of the Modal codebase, to fix some bugs on the
onDismissevent and to make this event available for Android as well. I do think that your contribution is a valid use case for iOS. Do you think it would be possible to adapt your changes on top of that update?
Absolutely. I'll refactor it.
Hi @ChrisSchofieldCheckatrade, thank you again to work on the PR.
The code looks good to me but, If I understand correctly, it is only working in the Old Architecture.
Have you tried with the New Architecture also? If not, could you perhaps try to implement the same behavior there as well? 🙏
Thank you so much!
I'll check that out and update here once done.
@cipolleschi All done, CI was passing I've just had to write a new commit to revert to not using src dir for specs as per: this commit: https://github.com/facebook/react-native/commit/5e8ce6ca5ee9274a94068994c145f2fc606a1fc8 and subsequent revert: https://github.com/facebook/react-native/commit/c92f7e5cc7d1796de8f3b7a3493ad84562319363
edit: and subsequent reintroduction: https://github.com/facebook/react-native/commit/26e33a5f175e4a715d827be773efdb427ca2b52d 😅
Hi @ChrisSchofieldCheckatrade, just an update not to keep you hanging. Last week we landed the change on modal for which I asked you to rebase and update this PR. Than, over the weekend, we had to revert the commit because it was causing issues internally. Now, I'm about to reland a lighter version of the previous change.
The root problem is that, for internal reasons, we can't land code that changes both the Native and the JS side at the side time. We need these changes to be split in Native and JS, to make sure they will not cause any issue.
To set expectations, I can take care of that after the other changes lands, but it will take some weeks. I hope this is ok with your timelines.
@cipolleschi our situation is that we need this feature for something in our application, and so long as you guys intend to merge this, we can continue to simply patch react-native in our own code base.
I'm happy to continue to maintain this PR for the coming weeks if it'll help you! Just give me an update when you're happy with the changes on your end. 😀
@ChrisSchofieldCheckatrade many thanks for your understanding, I really appreciate it. I'll keep you in the loop to make this happen!
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Not for now, I think I can split it pretty easily, thanks! One of the two commit will still be attributed to you as contributor. If you want to have both the commit attributed to you, You could split the PR in Native and JS and I'll take care of make them depend on each other internally.
@ChrisSchofieldCheckatrade
Not for now, I think I can split it pretty easily, thanks! One of the two commit will still be attributed to you as contributor. If you want to have both the commit attributed to you, You could split the PR in Native and JS and I'll take care of make them depend on each other internally.
@ChrisSchofieldCheckatrade
I don't mind, whichever is easier. 😄