AdSizes for AdMob banner ads #402

RedBrogdon · 2018-03-01T00:29:45Z

This PR adds a new AdSize class to the firebase_admob plugin for use in making banners (see Issue #8098). All the standard sizes are supported (see this AdMob guide for a list).

Right now, the public API and Android implementation are worked out, with iOS and finishing touches to come. I'm putting it up early so people can have a look at the API.

phrohdoh · 2018-03-01T17:17:20Z

...rebase_admob/android/src/main/java/io/flutter/plugins/firebaseadmob/FirebaseAdMobPlugin.java

+  private void callLoadBannerAd(int id, Activity activity, MethodChannel channel, MethodCall call, Result result) {
+    String adUnitId = call.argument("adUnitId");
+    if (adUnitId == null || adUnitId.isEmpty()) {
+      result.error("no_unit_id", "a non-empty adUnitId was not provided for ad id=" + id, null);


This wording is a bit confusing. Would you be open to changing this to something like a null or empty adUnitId...?

HansMuller

Glad to see this final bit of the banner ad API!

Just a few comments about how ad sizes are represented.

HansMuller · 2018-03-01T18:40:22Z

packages/firebase_admob/lib/firebase_admob.dart

+  SmartBanner,
+}
+
+/// [AdSize] represents the size of a banner ad.


It would be useful to link to the relevant doc here; https://developers.google.com/admob/android/banner#banner_sizes I think

HansMuller · 2018-03-01T18:43:44Z

packages/firebase_admob/lib/firebase_admob.dart

+}
+
+/// [AdSize] represents the size of a banner ad.
+class AdSize {


You could just expose const constructors, like const AdSize.banner(), although it might be simpler to just expose statics, like AdSize.banner`.

I was actually dithering about the most Dart-like way to do this, so I'm glad you brought it up. If having pre-built instances hang out as static constants is allowed, that's my preference.

Although it doesn't matter here, static final class constants are computed lazily. Which is nice.

HansMuller · 2018-03-01T18:45:05Z

packages/firebase_admob/lib/firebase_admob.dart

+  /// to use one of the named constructors like [AdSize.banner].
+  AdSize({this.width, this.height})
+      : assert(width > 0),
+        assert(height > 0),


I don't think you want to expose this constructor, since all of the valid ad sizes are covered by the other constructors.

It should be a (private) const constructor where all of the parameters are @required.

Done. If the plugin is ever expanded to handle DoubleClick for Publishers, it'll need to be made public again, but this is just fine for AdMob.

HansMuller · 2018-03-01T18:49:34Z

packages/firebase_admob/lib/firebase_admob.dart

+  /// automatically adjust their width to match that of the displaying device's
+  /// screen. Height also varies, and will be 32, 50, or 90 depending on how
+  /// tall the screen is.
+  AdSize.smartBanner()


From the docs, it seems like there are only three valid height values (and you can't really specify the width?). You could either provide three constants or just provide a const constructor whose width could only be one of the 3 valid values.

Since the recommended practice is to base the banner's size on the screen size, you could also provide a smartBannerFor(MediaQueryData) constructor that DTRT.

I think I need to revise this comment, since it clearly didn't impart the right info. :)

If you choose "smart banner" as your ad size, there are no other inputs you're allowed to give. You, as the publisher, do not determine how wide or high the ad is. The SDK will automatically set the width of the AdView to match the device width, and will set its height based on a calculation involving device height. So there's nothing more for the API to do here other than give the publisher a way to say "make me an AdSize object of the smart banner type."

Sorry, I just didn't read this stuff carefully enough.

You shouldn't have to, though, so it's good feedback.

HansMuller · 2018-03-01T18:50:44Z

packages/firebase_admob/lib/firebase_admob.dart

  /// Create a BannerAd.
  ///
  /// A valid [adUnitId] is required.
  BannerAd({
    @required String adUnitId,
+    @required this.size,


Why not give this a default value?

There are two reasons, I guess.

Our Android and iOS APIs don't have a default, so it's what I'm used to.

In my mind, publishers should be consciously picking a size based on what their UI looks like. Most will pick the standard 320x50 banner anyway, but I'd like for them to think at least for a moment about it.

I see, sounds reasonable.

HansMuller · 2018-03-01T18:59:13Z

packages/firebase_admob/lib/firebase_admob.dart

@@ -82,6 +82,65 @@ class MobileAdTargetingInfo {
  }
 }

+// The types of ad sizes supported for banners.
+enum AdSizeType {


It looks like indices of these enums are significant. That's worth a '//' implementation comment.

Should I just add to the comment that's there and indicate how I'm using the index values? If there's an example of a similar comment, feel free to point me to it.

https://docs.flutter.io/flutter/scheduler/SchedulerPhase-class.html uses an enum's index this way

It feels a little brittle to rely on the index because adding another id could disturb things. On the other hand, you could easily write a regression test that checked this

Makes sense. What about passing the String name of the value, then, rather than using the index?

mit-mit · 2018-03-02T08:45:52Z

Thanks much @RedBrogdon for the contribution! Looks like there is a small merge conflict with master. Can you take a quick look?

Purus · 2018-03-03T17:51:15Z

Please let me know I can be of any help on this?
Also kindly check on the issue flutter/flutter#14222 if you can help on this.

googlebot · 2018-03-07T18:55:33Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

RedBrogdon · 2018-03-08T19:07:04Z

The API is finished at this point, and it's implemented all the way down for iOS and Android. Tests are passing, and the code's linted.

PTAL!

Purus · 2018-03-09T05:27:32Z

Hope it's merged soon :)

HansMuller

LGTM

HansMuller · 2018-03-09T18:24:34Z

...rebase_admob/android/src/main/java/io/flutter/plugins/firebaseadmob/FirebaseAdMobPlugin.java

@@ -38,14 +39,61 @@ private FirebaseAdMobPlugin(Registrar registrar, MethodChannel channel) {
  private void callInitialize(MethodCall call, Result result) {
    String appId = call.argument("appId");
    if (appId == null || appId.isEmpty()) {
-      result.error("no_app_id", "a non-empty AdMob appId was not provided", null);
+      result.error("no_app_id", "a null or empty AdMob appId was provided", null);


RedBrogdon · 2018-03-09T21:57:02Z

@Purus, to answer your earlier question about what you might do to help: please give this a thorough test in whatever apps you're working on. It's playing along just fine for me, but I'm sure there are edge cases I haven't discovered.

Purus · 2018-03-10T07:10:38Z

Sure.. I am happy to test it in my development apps. I will test once this is merged.

RedBrogdon · 2018-03-19T21:25:00Z

When is this one likely to get merged?

HansMuller · 2018-03-19T21:42:11Z

Sorry about the delay, landing and publishing this PR now

* commit 'a404fbfd4842b51b57bc0c6298604cf5674e2c02': (40 commits) Update README.md (flutter#430) Updated to 0.5.0 (flutter#428) AdSizes for AdMob banner ads (flutter#402) bump minor version and add changelog entry (flutter#427) add fetchProvidersForEmail wrapper (flutter#410) Incremental Build Script (flutter#422) add runtime permission requests for external storage and camera (flutter#424) Fix Dart 2 type error and deprecation (flutter#425) Fix a Dart 2 runtime cast failure in firebase_database test (flutter#419) Fixed deprecation warnings (flutter#420) Fix Dart 2 runtime error in the camera plugin (flutter#417) Always use the latest Flutter available (flutter#415) Spelling (flutter#411) Configure Flutter CI (flutter#383) Set SDK constraints to match Flutter beta (flutter#412) Remove Gradle artifacts from repo (flutter#414) Google maps plugin stub (flutter#405) Allow null document snapshot data when document does not exist (flutter#406) Fix new formatting errors (flutter#408) Fix url_launcher for iOS <10 (flutter#407) ...

Adds a new AdSize class to the firebase_admob plugin for use in making banners (see Issue #8098). All the standard sizes are supported.

Adding AdSize for banners. Still more work coming.

a74ebb7

googlebot added the cla: yes label Mar 1, 2018

RedBrogdon mentioned this pull request Mar 1, 2018

"Unable to merge dex" error with firebase_admob package flutter/flutter#14874

Closed

phrohdoh reviewed Mar 1, 2018

View reviewed changes

HansMuller reviewed Mar 1, 2018

View reviewed changes

googlebot added cla: no and removed cla: yes labels Mar 7, 2018

Merging, changes for review.

0ee41f5

RedBrogdon force-pushed the adsizes branch from 2410057 to 0ee41f5 Compare March 7, 2018 21:29

RedBrogdon and others added 5 commits March 7, 2018 13:34

Merge branch 'master' into adsizes

3e712b5

Adds iOS implementation of ad sizes.

561cb59

Formatting new AdMob plugin code.

ebeb16f

Further linting of firebase_admob.

1809960

Correcting firebase_admob test.

0abe4b7

RedBrogdon changed the title ~~[WIP] AdSizes for AdMob banner ads~~ AdSizes for AdMob banner ads Mar 8, 2018

HansMuller approved these changes Mar 9, 2018

View reviewed changes

collinjackson assigned kroikie Mar 13, 2018

HansMuller merged commit fe2f653 into flutter:master Mar 19, 2018

HansMuller mentioned this pull request Mar 19, 2018

Updated to 0.5.0 #428

Merged

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

AdSizes for AdMob banner ads (flutter#402)

88adb30

Adds a new AdSize class to the firebase_admob plugin for use in making banners (see Issue #8098). All the standard sizes are supported.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AdSizes for AdMob banner ads #402

AdSizes for AdMob banner ads #402

RedBrogdon commented Mar 1, 2018 •

edited

Loading

phrohdoh Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller left a comment

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

HansMuller Mar 1, 2018

RedBrogdon Mar 1, 2018

mit-mit commented Mar 2, 2018

Purus commented Mar 3, 2018

googlebot commented Mar 7, 2018

RedBrogdon commented Mar 8, 2018

Purus commented Mar 9, 2018

HansMuller left a comment

HansMuller Mar 9, 2018

RedBrogdon commented Mar 9, 2018

Purus commented Mar 10, 2018

RedBrogdon commented Mar 19, 2018

HansMuller commented Mar 19, 2018

AdSizes for AdMob banner ads #402

AdSizes for AdMob banner ads #402

Conversation

RedBrogdon commented Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HansMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mit-mit commented Mar 2, 2018

Purus commented Mar 3, 2018

googlebot commented Mar 7, 2018

RedBrogdon commented Mar 8, 2018

Purus commented Mar 9, 2018

HansMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBrogdon commented Mar 9, 2018

Purus commented Mar 10, 2018

RedBrogdon commented Mar 19, 2018

HansMuller commented Mar 19, 2018

RedBrogdon commented Mar 1, 2018 •

edited

Loading