-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Please add I-Mobile support to amp-ad #2470
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Hello we signed the CLA as a corporation, would you please have it double checked? We would be glad to provide you further information if you want. Our company name: i-mobile Co.,Ltd |
CLAs look good, thanks! |
Thank you very much for confirming! Looking forward to your feedbacks |
hi @cramforce would you please have our repo reviewed? Thanks a lot. |
@@ -34,6 +34,11 @@ export const adPrefetch = { | |||
yieldmo: 'https://static.yieldmo.com/ym.amp1.js', | |||
revcontent: 'https://labs-cdn.revcontent.com/build/amphtml/revcontent.amp.min.js', | |||
teads: 'https://cdn.teads.tv/media/format/v3/teads-format.min.js', | |||
imobile: [ | |||
'https://spamp.i-mobile.co.jp/script/amp.js', |
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.
If you load all of these, why not just load one file?
Looks good. Some small comments. Please rebase against master and squash commits. |
@cramforce Thank you very much for your quick reply. We have fixed the problems issued by you and squashed the commits. Looking forward to hearing from your feedback soon, thanks again. |
Looks great. This is ready to merge, but please rebase against master and repush. There are currently merge conflicts. I believe one of the files you edited was deleted. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
Please squash commits. |
…variables and require functions for including gulp plugins.
* Adding postcss-import to the project * Adding amp-social-share extension
This blackholes them since nobody should ever use these in an ad.
@jridgewell is working on the real fix.
Updated amp-fill-content to workaround IOS This commit addresses the issue described here: #2491
add readme jslint fix 1. add test 2. fix readme small fix based on AMP manager's review conflict resloved conflict resloved conflict resloved another conflict found
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
@cramforce Sorry we messed something up. We will create a new repo and resend out pull request ASAP. Sorry for the inconvenience. |
Please add support to amp-ad for the imobile type.
Our CLA: i-mobile Co.,Ltd
Looking forward to hearing from your response!