Skip to content
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

Google Admob integration #92

Merged
merged 11 commits into from Dec 28, 2016

Conversation

Projects
None yet
3 participants
@wajahatch888
Copy link
Contributor

wajahatch888 commented Dec 26, 2016

This update can support both Banner and Interstitial ads. You can integrate ads with below json in body tag.

"ads": {
            "admob": [
              {
                "type": "banner",
                "unitId": "a14dccd0fb24d45"
              },
              {
                "type": "interstial",
                "unitId": "ca-app-pub-6606303247985815/7014816684"
              }
            ]
          }

@wajahatch888 wajahatch888 referenced this pull request Dec 26, 2016

Closed

feature request: admob #79

@lukeramsden

This comment has been minimized.

Copy link
Contributor

lukeramsden commented Dec 26, 2016

I have to say @wajahatch888 , you're really good at this 😂

@gliechtenstein

This comment has been minimized.

Copy link
Contributor

gliechtenstein commented Dec 26, 2016

@wajahatch888 wow that was fast! Looking through the code I have a couple of questions (Please note that I'm a newbie with ads so some questions may sound clueless)

  1. How does the frame work for Ads? I'm especially curious about the part where it says CGRect frame = CGRectMake(0, self.view.frame.size.height - GAD_SIZE_468x60.height, GAD_SIZE_468x60.width, GAD_SIZE_468x60.height); Does this automatically resize to fit the width of the device?

  2. I noticed the part where it takes into account the footer height. Have you tested with footer.input? (There are two types of footer at the moment--tabs and input)

  3. Related to #2, what happens when the user focuses the input? This part may be a bit tricky, since the keyboard will go up and we probably need to recalculate the frame of the ad.

  4. Is it possible to have multiple heterogeneous ad types in one screen? For example, one Ad from AdMob, but another from another provider. Or even one interstitial from AdMob and another banner ad from Admob. The current implementation looks like it supports a single type at a time, and I am wondering if we may ever need to support multiple simultaneous ad types in the future (or if this is even possible at all). Another reason I ask is because there can be different ways of representing this and was wondering what the most adequate version would be.

    For example, the current json markup spec will work well if there can be only a single ad provider in one screen at a time. This type of representation works well when a single type can exist at a time, just like how there can be only a single footer type at a time (either input or tabs but shouldn't be using both simultaneously).

    On the other hand if there can be multiple types at once, we could also use something like:

{
 "ads": [{
   "type": "admob",
   "options": {
     "type": "banner",
     "unitId": "a14dccd0fb24d45"
   }
 }, {
   "type": "admob",
   "options": {
     "type": "interstial",
     "unitId": "ca-app-pub-6606303247985815/7014816684"
   }
 }]
}

This approach would be analogous to how we use sections or layers (They're arrays of objects that represent a unit)

But like I said, I'm a total newbie with ads and this really depends on the possibilities going forward, so I would appreciate your insight.

@wajahatch888

This comment has been minimized.

Copy link
Contributor Author

wajahatch888 commented Dec 27, 2016

@gliechtenstein , Thanks for reviewing. Below are the replies for your concerns

1- Previously ads were not showing with full width. Full width compatibility added.
2- Thanks for letting me know about footer input. i was unaware with that and was assuming that we only have tab footer..Compatibility added for chat footer.
3- Ads are going along with keyboard :)
4- Previous commit has the power to show both types of ads simultaneously or standalone. In this i commit i just changed the structure of json as you suggested above,

{
 "ads": [{
   "type": "admob",
   "options": {
     "type": "banner",
     "unitId": "a14dccd0fb24d45"
   }
 }, {
   "type": "admob",
   "options": {
     "type": "interstial",
     "unitId": "ca-app-pub-6606303247985815/7014816684"
   }
 }]
} 

Ads are working on Portrait and Landscape mode.

@gliechtenstein

This comment has been minimized.

Copy link
Contributor

gliechtenstein commented Dec 27, 2016

Awesome. Just re-read through the code and I think the code is good to go!

That said, I did run into a critical problem that I hadn't thought of before. The problem is:

Problem

You know how Apple asks you if you have used Advertising Identifier? Due to the way Jasonette works, if we just ship it this way, everyone who uses Jasonette must sign "YES" to this question.

idfa

To tackle this problem we need to make sure that the code for the ads IS NOT included in the resulting binary by default unless specified.

There was a similar situation earlier when we implemented push notification because not everyone needs the push feature yet this caused problems when submitting--the appstore would complain that there's a code for push notification but you haven't enabled the push capability from the capabilities tab in XCode.

How we tackled a similar problem with push notifications

The solution back then was to use #ifdef to selectively include the related code so that only those who want to implement push notifications would turn it on and use it. Basically, to use push notifications you would open Constants.h and uncomment the define statement so PUSH becomes true. And then you turn on the push capability from the capabilities tab and you're set to go.

Here's the related thread (The last comment in the thread is the solution): #53
And here's the related commit: debb04a


This is just one idea--theoretically we could do the same by adding another #define called AD or something so we can selectively include/exclude the corresponding code (exclude by default)--but this was just a quick idea I had come up with and I'm open to any new ideas. If you have another idea we should discuss. Please let me know!

@wajahatch888

This comment has been minimized.

Copy link
Contributor Author

wajahatch888 commented Dec 28, 2016

@gliechtenstein , I just updated the code with #ifdef condition. Please review it.

Wajahat Chaudhry added some commits Dec 28, 2016

@gliechtenstein gliechtenstein merged commit 689d7da into Jasonette:develop Dec 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.