-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
feat(banner): Support maximum height for Inline Adaptive banners #663
base: main
Are you sure you want to change the base?
Changes from all commits
7c825ff
0f15efe
c6d7303
5cdd8a7
c307c19
caf26dc
ab22522
7f6c3a8
c308c6b
1f09ec7
34f3f90
c3b75bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,11 +135,21 @@ public void setSizes(ReactNativeAdView reactViewGroup, ReadableArray value) { | |
payload.putDouble("height", adSize.getHeight()); | ||
sendEvent(reactViewGroup, EVENT_SIZE_CHANGE, payload); | ||
} | ||
|
||
reactViewGroup.setSizesArray(value); | ||
reactViewGroup.setSizes(sizeList); | ||
reactViewGroup.setPropsChanged(true); | ||
} | ||
|
||
@ReactProp(name = "maxAdHeight") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
public void setMaxAdHeight(ReactNativeAdView reactViewGroup, float value) { | ||
reactViewGroup.setMaxAdHeight(value); | ||
ReadableArray adSizes = reactViewGroup.getSizesArray(); | ||
if (adSizes != null) { | ||
this.setSizes(reactViewGroup, adSizes); | ||
} | ||
Comment on lines
+147
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my testing in the example app, this condition is never true. So we might not need the extra sizesArray. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, I cannot reproduce this in testing, but I can confirm it does happen in the other order sometimes (and cause crashes without the null check) when you run it at scale in production. |
||
reactViewGroup.setPropsChanged(true); | ||
} | ||
|
||
@ReactProp(name = "manualImpressionsEnabled") | ||
public void setManualImpressionsEnabled(ReactNativeAdView reactViewGroup, boolean value) { | ||
reactViewGroup.setManualImpressionsEnabled(value); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,8 +66,13 @@ - (void)setUnitId:(NSString *)unitId { | |
|
||
- (void)setSizes:(NSArray *)sizes { | ||
__block NSMutableArray *adSizes = [[NSMutableArray alloc] initWithCapacity:sizes.count]; | ||
_sizeStrings = sizes; | ||
CGFloat maxAdHeight = -1; | ||
if (_maxAdHeight > 0) { | ||
maxAdHeight = _maxAdHeight; | ||
} | ||
[sizes enumerateObjectsUsingBlock:^(id jsonValue, NSUInteger idx, __unused BOOL *stop) { | ||
GADAdSize adSize = [RNGoogleMobileAdsCommon stringToAdSize:jsonValue]; | ||
GADAdSize adSize = [RNGoogleMobileAdsCommon stringToAdSize:jsonValue withMaxHeight: maxAdHeight]; | ||
if (GADAdSizeEqualToSize(adSize, GADAdSizeInvalid)) { | ||
RCTLogWarn(@"Invalid adSize %@", jsonValue); | ||
} else { | ||
|
@@ -78,6 +83,14 @@ - (void)setSizes:(NSArray *)sizes { | |
_propsChanged = true; | ||
} | ||
|
||
- (void)setMaxAdHeight:(CGFloat)maxAdHeight { | ||
_maxAdHeight = maxAdHeight; | ||
if (_sizeStrings != nil) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I couldn't reproduce this on iOS or Android, but I was missing the same check on Android initially and running this with ~25k (out of ~700k) Android users in production, I was getting 10s of crashes a day. My plan is to refactor this to pass an object with the size related props across to native in a single prop, while having separate props exposed on the JS side. |
||
[self setSizes: _sizeStrings]; | ||
} | ||
_propsChanged = true; | ||
} | ||
|
||
- (void)setRequest:(NSString *)request { | ||
NSData *jsonData = [request dataUsingEncoding:NSUTF8StringEncoding]; | ||
NSError *error = nil; | ||
|
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.
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.
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.
that has been bugging me for a long long time 😆 - thanks for cleaning that up