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.

Sign up for GitHub

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

Provide Compatibility for Google Playable Ads #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Provide Compatibility for Google Playable Ads #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion config.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"external_url_prefix": ""
},
"mraid_support": false,
"snapchat_cta": false,
"cta_option": false,
"adOrientation": false,
Copy link
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
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!

"compress_engine": false
}
}
38 changes: 30 additions & 8 deletions one-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,14 +333,6 @@ function inlineAssets(projectPath) {
fs.writeFileSync(location, contents);
})();

// Add Snapchat CTA code if needed
(function() {
if (config.one_page.snapchat_cta) {
console.log("↪️ Adding Snapchat Ad CTA code");
addLibraryFile('snapchat-cta.js');
}
})();

// 9. Compress the engine file with lz4
(function() {
if (config.one_page.compress_engine) {
Expand Down Expand Up @@ -373,6 +365,36 @@ function inlineAssets(projectPath) {
}
})();

// Add Snapchat/Google CTA code if needed
Copy link
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
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
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

(function() {
switch(config.one_page.cta_option) {
case 'snapchat':
console.log("↪️ Adding Snapchat Ad CTA code");
addLibraryFile('snapchat-cta.js');
break;
case 'google':
console.log("↪️ Adding Google Ad CTA code");
var allowedValues = ["portrait", "landscape", "portrait,landscape"];
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';
Comment on lines +378 to +380
Copy link
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
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
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

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
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
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?


indexContents = indexContents.replace(
'<head>',
`<head>\n ${exitApi + googlePrimaryAssets}`
);
}

else {
console.log('Error: Invalid Google Ad Orientation supplied');
}

Copy link
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
Author

Choose a reason for hiding this comment

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

Will do!

break;
}
})();


// 10. Replace references to all scripts in index.html with contents of those files.
// 11. Replace playcanvas-stable.min.js in index.html with a base64 string of the file.
Expand Down