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

Please add I-Mobile support to amp-ad #2470

Closed
wants to merge 116 commits into from
Closed

Please add I-Mobile support to amp-ad #2470

wants to merge 116 commits into from

Conversation

imobiletmp
Copy link
Contributor

Please add support to amp-ad for the imobile type.
Our CLA: i-mobile Co.,Ltd

Looking forward to hearing from your response!

@googlebot
Copy link

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@imobiletmp
Copy link
Contributor Author

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
Our CLA: i-mobile Co.,Ltd

@googlebot
Copy link

CLAs look good, thanks!

@imobiletmp
Copy link
Contributor Author

Thank you very much for confirming! Looking forward to your feedbacks

@imobiletmp imobiletmp closed this Mar 7, 2016
@imobiletmp imobiletmp reopened this Mar 7, 2016
@imobiletmp
Copy link
Contributor Author

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',
Copy link
Member

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?

@cramforce
Copy link
Member

Looks good. Some small comments. Please rebase against master and squash commits.

@imobiletmp
Copy link
Contributor Author

@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.

@cramforce
Copy link
Member

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.

@googlebot
Copy link

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot
Copy link

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.

@cramforce
Copy link
Member

Please squash commits.

Meggin and others added 26 commits March 11, 2016 10:30
* Adding postcss-import to the project
* Adding amp-social-share extension
This blackholes them since nobody should ever use these in an ad.
And robustly handle the case that a Tweet has been deleted.

Primary issue of #2530

Files #2536 because I realized we have no tests for the twitter widget.
@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
@googlebot
Copy link

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@imobiletmp
Copy link
Contributor Author

@cramforce Sorry we messed something up. We will create a new repo and resend out pull request ASAP. Sorry for the inconvenience.

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.