feat(banner): Support maximum height for Inline Adaptive banners

[markwilcox]

Description

Inline adaptive banners are the best variety to use on cell-based layouts like list views. They can also maximise revenue on scrollViews by taking advantage of more available inventory at different sizes. However they can grow very tall and ruin the UX in these situations.

The native iOS and Android SDKs support creating an inline adaptive banner with a maximum height to solve this issue. This PR is intended to add support for that behaviour from React Native.

Related issues

Fixes #541

Release Summary

Add support for optional maxHeight property on BannerAd

Checklist

• I read the Contributor Guide
  and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in __tests__e2e__
  • jest tests added or updated in __tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

I couldn't see any relevant tests to add to in e2e or tests. The new prop is optional and only has any effect on an inline adaptive banner. I went for forgiving - e.g. setting the prop on the wrong banner format is ignored, setting a prop smaller than the minimum supported by the native SDKs automatically increases it to the minimum size (32px).

I added a test in the example app and ensured it worked there on old and new architectures. Let me know if you want any other tests added, or to be more strict validating use of the prop (e.g. against minimum size, or in combination with other ad formats).

I'm not 100% on this implementation because of the way we need both the sizes prop and the maxHeight prop to set a size in the native ad request. As far as I know the order of calling these isn't guaranteed, so I'm storing the original sizes array, and recalculating the sizes if the maxHeight prop gets set on the native side. It works, but I wonder if it will ultimately create 2 requests for each ad shown and impact the show rate? If you can suggest a better implementation strategy without changing the API too much I'm willing to give it a go.

If you think this approach is OK then I'll update the docs.

🔥

Think react-native-google-mobile-ads is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow Invertase on Twitter

[docs-page]

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/react-native-google-mobile-ads~663

Documentation is deployed and generated using docs.page.

[CLAassistant]

All committers have signed the CLA.

[markwilcox]

This was a quick hack because there are too many tests to fit on a screen already and they're all 100% pressable area, so you can't scroll the scroll view. This gives you some space on the right to scroll it. Happy to remove, but I found it helpful.

[mikehardy]

that has been bugging me for a long long time 😆 - thanks for cleaning that up

[markwilcox]

I wanted to call the native prop "maxHeight" but that ends up setting the native view max height too, which isn't what you want in the case where the user sets a height lower than the minimum (32px) because you get a minimum height ad back and then the view crops it.

[markwilcox]

How would you prefer a constant defined for this magic number?

[markwilcox]

Again here, how would you prefer a constant defined for the magic number?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.79%. Comparing base (a34c7ba) to head (5cdd8a7).
Report is 83 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
- Coverage   43.72%   42.79%   -0.93%     
==========================================
  Files          30       30              
  Lines         549      575      +26     
  Branches      151      151              
==========================================
+ Hits          240      246       +6     
- Misses        309      329      +20     

[dylancom]

Hi, thanks for the PR. I am not sure about the potential double requests. Also the extra "sizeStrings" array condition isn't very clear.

My current thinking is that we adapt the current size prop and create some helper to provide a max height:
<Banner size={getInlineAdaptiveBannerAdSize(320, 100)} />.
The helper method outputs a string that stringToAdSize (with small changes) will understand. So it could become something like: "INLINE_ADAPTIVE_BANNER_300x100" || "INLINE_ADAPTIVE_BANNER_300_100".

The Flutter implementation for e.g. has this methods:

• AdSize.getCurrentOrientationInlineAdaptiveBannerAdSize(int width)
• AdSize.getInlineAdaptiveBannerAdSize(int width, int maxHeight)

@mikehardy what do you think?

[markwilcox]

My current thinking is that we adapt the current size prop and create some helper to provide a max height: <Banner size={getInlineAdaptiveBannerAdSize(320, 100)} />. The helper method outputs a string that stringToAdSize (with small changes) will understand. So it could become something like: "INLINE_ADAPTIVE_BANNER_300x100" || "INLINE_ADAPTIVE_BANNER_300_100".

Currently from React Native we can't set the width anywhere on the adaptive banners as it's hardcoded to use the screen width.

If I was going to change the API I'd change that too. Maybe keep the JS API but allow separate width and maxHeight to be passed, then make them all a single object with the size string that gets passed across the bridge (or set as a single prop in the new architecture)?

Going for conversion to strings on the JS side and then parsing those strings feels a bit messy. The alternative I guess would be to go for minimal change and only support a fixed set of heights. Probably revenue vs. UX impact the only sizes that will really matter for height restriction are 50px, 100px & 250px.

We'll probably experiment with sizes on the fork I've made in production (millions of monthly impressions) so I can answer some questions about double requests (ideally the native SDKs would cancel in-flight requests if you made another one immediately, but no idea if they do) / show rate, and eCPM impact at different sizes empirically.

[mikehardy]

🤔

Currently from React Native we can't set the width anywhere on the adaptive banners as it's hardcoded to use the screen width.

If I was going to change the API I'd change that too. Maybe keep the JS API but allow separate width and maxHeight to be passed, then make them all a single object with the size string that gets passed across the bridge (or set as a single prop in the new architecture)?

That was surprising to read for me. I don't use this package so I am a maybe-interesting combination of not that knowledgeable but also interested in helping the package do well. "maybe-interesting" because I have a fresh set of eyes for most of these things and even I understand that control over the UX in design is vital, and that means ability to constrain width and height, yes? Thus, surprising we don't permit real control here

Looking at Flutter implementation, it's in there - width and height are fundamental on the size supertype:

https://github.com/googleads/googleads-mobile-flutter/blob/6aa897e3dcc2b2d97f4df61192b015388ce876d6/packages/google_mobile_ads/android/src/main/java/io/flutter/plugins/googlemobileads/FlutterAdSize.java#L22-L25

...some types have extra stuff like orientation and what not, and they have a lot of machinery in there for factory constructor type things, but width and height are controllable

We...do not have that based on what I see, right?

react-native-google-mobile-ads/src/BannerAdSize.ts

Line 19 in 2aa29f0

export enum BannerAdSize {

So, I'd say this is a worthy API change as well. Backwards-compatible or not - if I understand things correctly it seems widthxheight control is critical and adding them would be "doing it right"

Going for conversion to strings on the JS side and then parsing those strings feels a bit messy

Agreed there - that seems like the wrong way to go vs allowing width and height to be specifiable first-class and passed separately from JS to native

[mikehardy]

Even the Flutter example for inline adaptive takes great care to determine the size (in their case, meaning width) minus the gutters. It's so important to control the width they show it in their "here is our most basic imagined use so you can just try it" example

[dylancom]

Going for conversion to strings on the JS side and then parsing those strings feels a bit messy. The alternative I guess would be to go for minimal change and only support a fixed set of heights. Probably revenue vs. UX impact the only sizes that will really matter for height restriction are 50px, 100px & 250px.

I agree, that suggestion came from using what is already available in this lib (you can pass sizes such as "300x200")
which are old ideas now:

Size: BannerAdSize | string;
_The size of the banner. Can be a predefined size via `BannerAdSize` or custom dimensions, e.g. `300x200`_

So now is a great time to adjust our current api.
My personal preference goes to keep using the size prop we already have instead of adding additional props.
This is also how Flutter does it and I think we can take notes there.

Flutter example:

AdSize size = AdSize.getCurrentOrientationInlineAdaptiveBannerAdSize(
        _adWidth.truncate());`
        
// If you wish to limit the height of the banner, you can use the static method
// AdSize.getInlineAdaptiveBannerAdSize(int width, int maxHeight)

BannerAd(
      adUnitId: 'ca-app-pub-3940256099942544/9214589741',
      size: size,
      request: AdRequest()
)

[markwilcox]

So now is a great time to adjust our current api. My personal preference goes to keep using the size prop we already have instead of adding additional props. This is also how Flutter does it and I think we can take notes there.

Flutter example:

AdSize size = AdSize.getCurrentOrientationInlineAdaptiveBannerAdSize(
        _adWidth.truncate());`
        
// If you wish to limit the height of the banner, you can use the static method
// AdSize.getInlineAdaptiveBannerAdSize(int width, int maxHeight)

BannerAd(
      adUnitId: 'ca-app-pub-3940256099942544/9214589741',
      size: size,
      request: AdRequest()
)

The Flutter API is doing a lower level wrapper around the native SDK AdSize creation methods. To me that seems like an unnecessary extra round trip to native land for a trivial bit of config. Generating the value of a prop in native land and then passing it back again isn't really "the React Native way" in my experience.

We could replicate that without the extra round trip by calling a helper function that's pure JS and just constructs an object with the necessary params to pass across to native. Or we could just have separate (optional) props on the BannerAd itself that get converted to the appropriate object to pass to native. I'd be OK with going for either of those. I think separate optional props feels more React-like as an API, but the helper function is closer to the native SDK & Flutter APIs if consistency there is more helpful.

If there's a consensus on which one to go for I can update the PR. Might need a couple of weeks to prioritise it again.

[dylancom]

Oke so my first preference went towards bundling all size related things in the "size" prop.
But I am fine with an extra width and maxHeight prop.

Thanks for contributing!

[dylancom]

Suggested change

	if (maxHeight > 0) {
	if (maxHeight < 32) {
	return GADInlineAdaptiveBannerAdSizeWithWidthAndMaxHeight(viewWidth, 32.0);
	} else {
	return GADInlineAdaptiveBannerAdSizeWithWidthAndMaxHeight(viewWidth, maxHeight);
	}
	}
	if (maxHeight > 0) {
	return GADInlineAdaptiveBannerAdSizeWithWidthAndMaxHeight(viewWidth, MAX(maxHeight, 32.0));
	}

[dylancom]

I believe you can do something like:

Suggested change

	if (maxAdHeight > 0) {
	if (maxAdHeight < 32) {
	return AdSize.getInlineAdaptiveBannerAdSize(adWidth, 32);
	} else {
	return AdSize.getInlineAdaptiveBannerAdSize(adWidth, Math.round(maxAdHeight));
	}
	}
	if (maxAdHeight > 0) {
	return AdSize.getInlineAdaptiveBannerAdSize(adWidth, Math.round(Math.max(maxAdHeight, 32)));
	}
	return AdSize.getCurrentOrientationInlineAdaptiveBannerAdSize(
	reactViewGroup.getContext(), adWidth);

[dylancom]

From my testing in the example app, this condition never becomes truthy. So do we really need this extra sizeStrings?
It seems maxAdHeight is always set before sizes.
Android same?

At least in the example app I am always getting this order:

[dylancom]

From my testing in the example app, this condition is never true. So we might not need the extra sizesArray.

[dylancom]

Suggested change

	maxHeight?: number;
	/**
	* Limit inline adaptive banner height.
	* By default, inline adaptive banners instantiated without a maxHeight value have a maxHeight equal to the device height.
	*/
	maxHeight?: number;

[dylancom]

If we can't be sure that maxHeight is set before sizes we might group them in BaseAd: sizeConfig={{ sizes, maxHeight }}. So the native side receives them as a bundle but the user can just use seperate size and maxHeight props on the component. Thanks @mikehardy for the idea.