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

Remove custom dialog #117

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@geof90
Contributor

geof90 commented Dec 19, 2015

This PR upgrades to RN 0.17.0, and in so doing, removes our custom dialog module implementation since the OOTB RN Alert module is now cross platform.

Fixed a minor bug with tests, the remotePackage for InstallUpdateTests was exported under the "default" property and not directly as the exported object itself.

@@ -15,3 +15,46 @@
#-keepclassmembers class fqcn.of.javascript.interface.for.webview {
# public *;
#}

This comment has been minimized.

@geof90

geof90 Dec 19, 2015

Contributor

Just a note that this file change is auto-generated from running the react-native upgrade command.

@shishirx34

This comment has been minimized.

Contributor

shishirx34 commented Dec 21, 2015

LGTM!

@lostintangent

This comment has been minimized.

Member

lostintangent commented Dec 21, 2015

LGTM, but I'd like to address the few bugs we have logged before making a change in RN version dependency.

@lostintangent

This comment has been minimized.

Member

lostintangent commented Dec 28, 2015

Since this is simply a code-cleanup PR, which doesn't add any new features, but does represent a breaking change, I think we should wait to merge this along with adding assets support for Android, since that will require a new version of React Native as well. If your PR gets merged in time for 0.18.0, then I think it would be great to release a new minor release of the plugin that depends on that RN version, and adds in Android asset support and removes our dialog implementation at the same time.

@geof90

This comment has been minimized.

Contributor

geof90 commented Feb 2, 2016

Closed because of issues with the RN Alert being unable to support our scenario, such as it being dismissable when the user touches the outside of the dialog.

@geof90 geof90 closed this Feb 2, 2016

@lostintangent lostintangent deleted the remove-custom-dialog branch Apr 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment