-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
feat(alert): use support library for alert dialog #29832
Conversation
Hi @DomiR! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, 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. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Base commit: 02fed06 |
cc @dulmandakh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't know how to fix the BUCK files... circleci doesn't seem to build properly, but I compiled it on my machine and the result can be seen above. |
I can't wait for the next release and I'm scared to roll with my own fork for my app release 😅 Here https://github.com/DomiR/react-native-compat-alert is reimplementation of the module, that uses androidx.appcompat.app.AlertDialog. Would be nice to see this in the next release. |
Please remove everything in provided_deps, and add react_native_dep("third-party/android/androidx:appcompat") |
@dulmandakh there is still some BUCK file for the test build that needs updating... any ideas? |
You find a file that fails to compile, and usually there is a BUCK file in the same folder which might require change. |
This reverts commit dc14c91.
I dug a little bit, and couldn't a fix the test failure. It seems like AppCompat components require themed context, ThemedReactContext in RN case, but RN passes ReactApplicationContext to modules constructor which is used by DialogModule to show AlertDialog. Thus tests are failing. Please help @mdvacca |
Any updates? I have an app that is used a lot on devices with android 5/lollipop, this improvement would help me a lot. Thanks! |
Sucks that internal Facebook build tools are not the OS way. It builds for me, but BUCK needs to be configured differently to allow those tests to pass. In the meantime you can use my fork, which only replaces that import, with the one from androidx: https://github.com/DomiR/react-native-compat-alert |
What versions of Android does this aim to fix? |
I think Lollipop and earlier... other components also use the androidx compact lib, so it's not something unusual. Without compat the Alert works on older phones with the limitations mentioned above (no vertical button stacking, overflow on small devices). |
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 7 days with no activity. |
Summary
Instead of using the AlertDialog from the android library we use the AlertDialog from the support library to render dialogs properly on Lollipop phones.
Changelog
[Android] [Fixed] - Fix AlertDialog for older android devices by using AlertDialog from the androidx support library
Test Plan
Visual inspection.
With old AlertDialog:
With compat AlertDialog: