Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Moved AdMob init to firebase.init, added AdListener to showBanner() #476

Merged
merged 2 commits into from
Sep 8, 2017

Conversation

svzi
Copy link
Contributor

@svzi svzi commented Sep 2, 2017

The initialization of the AdMob plugin takes some time with current plugin-implementation. Showing the first banner could be delayed by a few seconds. Moving AdMob init from firebase.admob.showBanner() to firebase.init speeds that up a lot.

I've also added an AdListener to firebase.admob.showBanner(), similar to the one already used in firebase.admob.showInterstitial(). With that we can take care when a banner isn't displayed because of an network error or something else. The current implementation didn't return those kind of errors.

Copy link
Owner

@EddyVerbruggen EddyVerbruggen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, does the fact there's no iOS changes mean you didn't notice the delay there?

@@ -208,6 +208,9 @@ firebase.init = function (arg) {

var firebaseAuth = com.google.firebase.auth.FirebaseAuth.getInstance();

// init admob
com.google.android.gms.ads.MobileAds.initialize(appModule.android.context); // TODO not sure its bound to packagename.. this is from the admob-demo
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a user didn't enable AdMob? Let's make this conditional.

@svzi
Copy link
Contributor Author

svzi commented Sep 8, 2017

Hi!

Hi, does the fact there's no iOS changes mean you didn't notice the delay there?

No, that just mean that I'm only using Android for my project so far.

What if a user didn't enable AdMob? Let's make this conditional.

I would love to do that conditional, but I didn't find a way on how to detect if the user didn't enable AdMob. When you tell me how to check that, I would be happy to update my PR.

@EddyVerbruggen
Copy link
Owner

Would be great if you could update the PR!
I think a check like this should suffice:

  if (typeof(com.google.android.gms.ads) !== "undefined") {
    // init admob here
  }

@svzi
Copy link
Contributor Author

svzi commented Sep 8, 2017

I've updated my PR. I've also moved the code some rows down in the init, because I recognized that I added it into the lines of the auth init. So I moved it down.

@EddyVerbruggen EddyVerbruggen merged commit 7b59855 into EddyVerbruggen:master Sep 8, 2017
@EddyVerbruggen
Copy link
Owner

👍 👍 👍 Thanks!

@EddyVerbruggen EddyVerbruggen added this to the 4.0.7 milestone Sep 8, 2017
@svzi
Copy link
Contributor Author

svzi commented Sep 8, 2017

Thank you for building and maintaining this awesome plugin! Keep up the great work! 👍

@EddyVerbruggen EddyVerbruggen modified the milestones: 4.1.0, 4.0.7 Sep 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants