-
Notifications
You must be signed in to change notification settings - Fork 4k
[firebase_core] v2 embedding API #274
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
[firebase_core] v2 embedding API #274
Conversation
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! Left some comments. We should update version before merging in.
packages/firebase_core/example/android/app/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
packages/firebase_core/example/android/app/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
...firebase_core/android/src/main/java/io/flutter/plugins/firebase/core/FirebaseCorePlugin.java
Show resolved
Hide resolved
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. Thanks for including an integration test!
If you add an example/test_driver/firebase_core_e2e_test.dart, CI will eventually run the integration test on iOS also. But this isn't hooked up yet, so it's optional.
...firebase_core/android/src/main/java/io/flutter/plugins/firebase/core/FirebaseCorePlugin.java
Show resolved
Hide resolved
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.
Code itself LG, just some comments on AndroidX/Gradle changes.
...firebase_core/android/src/main/java/io/flutter/plugins/firebase/core/FirebaseCorePlugin.java
Outdated
Show resolved
Hide resolved
...firebase_core/android/src/main/java/io/flutter/plugins/firebase/core/FirebaseCorePlugin.java
Show resolved
Hide resolved
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 modulo one Javadoc nit.
Description
Fixes flutter/flutter#41869
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?