Skip to content

Provide Compatibility for Google Playable Ads#44

Open
eproasim wants to merge 1 commit into
playcanvas:mainfrom
eproasim:google-ad-compatibility
Open

Provide Compatibility for Google Playable Ads#44
eproasim wants to merge 1 commit into
playcanvas:mainfrom
eproasim:google-ad-compatibility

Conversation

@eproasim

Copy link
Copy Markdown
  • Added new config property of adOrientation for purposes of defining Google's ad.orientation and ad.size meta tags

  • Added inclusion of Google ExitApi script at the top of when google CTA is selected

  • Added ad.size and ad.orientation meta tags after ExitApi

  • Wrapped existing Snapchat CTA in a switch case based on "cta_option" value

  • Moved Snapchat CTA code to execute after engine has been compressed and patched

  • Changed "snapchat_cta" to "cta_option" accepting strings of "google" or "snapchat"

  • Changed config.template.json to reflect new available settings (adOrientation, cta_option)

- Added new config property of adOrientation for purposes of defining Google's ad.orientation and ad.size meta tags

- Added inclusion of Google ExitApi script at the top of <head> when google CTA is selected

- Added ad.size and ad.orientation meta tags after ExitApi

- Wrapped existing Snapchat CTA in a switch case based on "cta_option" value

- Moved Snapchat CTA code to execute after engine has been compressed and patched

- Changed "snapchat_cta" to "cta_option" accepting strings of "google" or "snapchat"

- Changed config.template.json to reflect new available settings (adOrientation, cta_option)
@eproasim

eproasim commented Nov 11, 2022

Copy link
Copy Markdown
Author

This is my first crack at getting a one-page output to work with Google's playable ad validator. I can confirm that it works wit the following settings in config,json:

{...

"one_page": {
        "patch_xhr_out": true,
        "inline_game_scripts": true,
        "extern_files": false,
        "mraid_support": false,
        "cta_option": "google",
        "adOrientation": "portrait",
        "compress_engine": true
    }
}

Please let me know what can be improved.

Comment thread config.template.json
"mraid_support": false,
"snapchat_cta": false,
"cta_option": false,
"adOrientation": false,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"adOrientation": false,
"ad_orientation": false,

To keep inline with naming of other properties

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that a few minutes after I did the commit, and figured this feedback would appear. Will update!

Comment thread one-page.js
}
})();

// Add Snapchat/Google CTA code if needed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason to move it from before step 9 to after?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only to have the meta tags appear at the top of the file. Visually, it made more sense to me, but I'm happy to return it to its original position.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which, the move is fine. Not a big deal and doesn't change functionality

Comment thread one-page.js
var orientation = config.one_page.adOrientation;
var size = (orientation == 'portrait') ? 'width=320,height=480' : 'width=480,height=320\n';
var googlePrimaryAssets = ` <meta name="ad.orientation" content="${orientation}">\n <meta name="ad.size" content="${size}">`;
var exitApi = `<script type="text/javascript" src="https://tpc.googlesyndication.com/pagead/gadgets/html5/api/exitapi.js"></script>\n`;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding it explicitly like this, it would be good to have a function like addLibraryFile instead so this logic is reusable for future networks

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand,

Ideally, a new, reusable function would be created at the top of inlineAssets() called something along the lines of 'createAdMetaTags' accepting the arguments, network, orientation, size(?), metaSizeName(?), metaOrientationName(?) ? This function would create the appropriate size and orientation meta tags, and based on the network, import any required API's and at it to the top of the head tag?

Comment thread one-page.js
Comment on lines +378 to +380
if(config.one_page.adOrientation && allowedValues.includes(config.one_page.adOrientation)) {
var orientation = config.one_page.adOrientation;
var size = (orientation == 'portrait') ? 'width=320,height=480' : 'width=480,height=320\n';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ad_orientation logic be outside the google CTA given it's a separate property in the config?

Happy to leave it here for now as I don't know if this is a common meta property outside of Google and we can always pull this out later

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also wondering about this. I almost changed cta_option to be an object based property, but it didn't feel right to have those settings embedded given that orientation and size don't directly relate to a CTA.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it as is for now, we can always change it later

Comment thread one-page.js
else {
console.log('Error: Invalid Google Ad Orientation supplied');
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a default with error messaging that the CTA option is not supported

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@playcanvas playcanvas deleted a comment Nov 27, 2022
@willeastcott

Copy link
Copy Markdown
Collaborator

Looks like this PR has been left hanging for quite some time. Are there still any plans for merging it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants