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

Multiple sizes implementation in AdMobBanner can be improved #26

Closed
edodusi opened this issue Sep 10, 2021 · 5 comments
Closed

Multiple sizes implementation in AdMobBanner can be improved #26

edodusi opened this issue Sep 10, 2021 · 5 comments

Comments

@edodusi
Copy link

edodusi commented Sep 10, 2021

When you want multiple sizes in AdManager Banner you call <GAMBannerAd> and pass the sizes attribute as an array.
However, the element has a mandatory size attribute: this is incorrect according to the SDK docs, in fact you should bu able to pass either a single size or an array of sizes.

The native implementation also leads to mixed results: the module calls requestAd() after both setSize() and setSizes(), but the requestAd() method only checks for the single size attribute to be non-null, so there's a race condition, if setSize gets called before setSizes you end up with a double call, one with a single size and one with multisize.

I needed the multisize feature for an app so I quickly fixed it for my own need, but I think the whole element should be refactored a bit to let a caller set multiple or single size and only perform one requestAd() call.

@wjaykim if you didn't already plan to change this behavior let me know and I will create a more robust PR soon as I can.

@wjaykim
Copy link
Collaborator

wjaykim commented Sep 10, 2021

Hi, I am aware of this and size prop will be changed to nullable in future version. Thank you for reporting

@edodusi
Copy link
Author

edodusi commented Sep 10, 2021

Thank you! Will check future releases for closing this one 😉

@wjaykim
Copy link
Collaborator

wjaykim commented Sep 11, 2021

New version 1.1.0 is released and changes related to this is included.

@edodusi
Copy link
Author

edodusi commented Sep 11, 2021

Thanks @wjaykim, just checked, I left you a comment on the related commit because you accidentally broke the types declaration file, anyway thanks for the great work!

@edodusi edodusi closed this as completed Sep 11, 2021
@edodusi
Copy link
Author

edodusi commented Sep 11, 2021

@wjaykim oops sorry I saw you fixed it in the next commit so nevermind 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants