✨ Remixd AMP ad-tag implementation #30030

glendza1 · 2020-08-28T18:18:23Z

The implementation of the ad-tag for the Remixd player.

google-cla · 2020-08-28T18:18:28Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

amp-owners-bot · 2020-08-28T18:18:32Z

Hey @ampproject/wg-caching! These files were changed:

validator/testdata/feature_tests/ads.html

honeybadgerdontcare

validator test file change lgtm

ads/remixd.js

ads/remixd.md

lgtm-com · 2020-08-28T18:40:03Z

This pull request introduces 1 alert when merging 3f5e0f2 into 084be74 - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

ads/_config.js

calebcordry · 2020-08-28T21:23:44Z

Thanks for contributing! Looks like you have a couple of lint errors. You can expose those using gulp lint, and try to fix using gulp lint --fix or check out more details here

Also I think you need to generate a new validator .out file (docs) for your new tag.

Let me know if you need help with the CLA, or feel free to ping me when you get it sorted.

google-cla · 2020-09-15T17:24:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

lgtm-com · 2020-09-15T17:47:54Z

This pull request introduces 1 alert when merging 2c4c9db into e7a2b26 - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

glendza1 · 2020-09-22T11:38:47Z

@googlebot I signed it!

google-cla · 2020-09-22T11:38:53Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

twifkak

@calebcordry Any other comments or approval blockers?

(Hi everybody. Just interjecting because we saw this during a bug scrub.)

ads/_config.js

ads/remixd.js

calebcordry · 2020-09-28T20:02:54Z

ads/remixd.js

+  const sriptVersion = data.version || '5';
+  const tagUrl = 'https://tags.remixd.com/player/v'+ sriptVersion +'/index.js?cb=' + Math.random();
+
+  document.write(


Please use loadScript or writeScript from 3p.js here

The reason we are not using writeScript is that the script we need to write needs to have an id set. I saw that writeScript doesn't have an option to set the element id, do you have any other suggestion or can we keep the code like this?

Yeah I guess not, so this is ok. Can you change it to use the passed in global.document though? Accessing document directly may do weird things based on different serving contexts.

calebcordry · 2020-09-28T20:03:50Z

Looks like we still need to sort out the CLA. @glendza let us know if you need assistance. Thanks!

google-cla · 2020-09-30T16:18:18Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

lgtm-com · 2020-09-30T16:40:20Z

This pull request introduces 1 alert when merging 0e5b11e into 9d1aeee - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

google-cla · 2020-10-02T15:44:58Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

glendza1 · 2020-10-02T15:49:55Z

The CLA was already signed by our CEO, under Remixd Media, Inc. Is there anything else we need to do?

lgtm-com · 2020-10-02T16:07:32Z

This pull request introduces 1 alert when merging db8c76f into 749b8de - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

googlebot · 2020-10-02T16:10:58Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

calebcordry · 2020-10-02T16:13:32Z

re: CLA it looks like your commits are fine, but there are a couple commits in this pr from an author with a nativo domain and a localhost domain that are not covered under the CLA

You also still have a couple lint errors, and will need to generate the new validator.out files to pass the CI tests.

* AMP tag implementation for the Remixd player * Linting/test fixes. * Bad character fix * Linting fixes

Marcelo Muniz added 2 commits August 11, 2020 05:28

- Initial commit for the amp support for remixd ad type.

5039dd7

- updated the URL so it works on production

3f5e0f2

amp-owners-bot bot requested review from alin04 and calebcordry August 28, 2020 18:18

google-cla bot added the cla: no label Aug 28, 2020

honeybadgerdontcare approved these changes Aug 28, 2020

View reviewed changes

ads/remixd.js Outdated Show resolved Hide resolved

ads/remixd.md Outdated Show resolved Hide resolved

calebcordry reviewed Aug 28, 2020

View reviewed changes

ads/_config.js Show resolved Hide resolved

honeybadgerdontcare added the WG: caching label Aug 31, 2020

milos added 2 commits September 14, 2020 09:02

Fixed the copyright year

4b19c3d

Implemented renderstart

2c4c9db

twifkak removed the request for review from alin04 September 28, 2020 19:53

twifkak reviewed Sep 28, 2020

View reviewed changes

ads/_config.js Show resolved Hide resolved

calebcordry reviewed Sep 28, 2020

View reviewed changes

Changed 'window' to 'global'.

0e5b11e

Added 'amp' indicatin for the player.

db8c76f

glendza1 closed this Oct 8, 2020

glendza1 deleted the remixd branch October 8, 2020 08:35

calebcordry pushed a commit that referenced this pull request Oct 15, 2020

✨ Remixd AMP ad-tag implementation #30030 (#30572)

5fa3930

* AMP tag implementation for the Remixd player * Linting/test fixes. * Bad character fix * Linting fixes

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020

✨ Remixd AMP ad-tag implementation ampproject#30030 (ampproject#30572)

8e8458f

* AMP tag implementation for the Remixd player * Linting/test fixes. * Bad character fix * Linting fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Remixd AMP ad-tag implementation #30030

✨ Remixd AMP ad-tag implementation #30030

glendza1 commented Aug 28, 2020

google-cla bot commented Aug 28, 2020

amp-owners-bot bot commented Aug 28, 2020 •

edited by ampprojectbot

honeybadgerdontcare left a comment

lgtm-com bot commented Aug 28, 2020

calebcordry commented Aug 28, 2020 •

edited

google-cla bot commented Sep 15, 2020

lgtm-com bot commented Sep 15, 2020

glendza1 commented Sep 22, 2020

google-cla bot commented Sep 22, 2020

twifkak left a comment

calebcordry Sep 28, 2020

glendza1 Sep 30, 2020

calebcordry Oct 2, 2020

calebcordry commented Sep 28, 2020

google-cla bot commented Sep 30, 2020

lgtm-com bot commented Sep 30, 2020

google-cla bot commented Oct 2, 2020

glendza1 commented Oct 2, 2020

lgtm-com bot commented Oct 2, 2020

googlebot commented Oct 2, 2020

calebcordry commented Oct 2, 2020

✨ Remixd AMP ad-tag implementation #30030

✨ Remixd AMP ad-tag implementation #30030

Conversation

glendza1 commented Aug 28, 2020

google-cla bot commented Aug 28, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

amp-owners-bot bot commented Aug 28, 2020 • edited by ampprojectbot

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Aug 28, 2020

calebcordry commented Aug 28, 2020 • edited

google-cla bot commented Sep 15, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

lgtm-com bot commented Sep 15, 2020

glendza1 commented Sep 22, 2020

google-cla bot commented Sep 22, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

twifkak left a comment

Choose a reason for hiding this comment

calebcordry Sep 28, 2020

Choose a reason for hiding this comment

glendza1 Sep 30, 2020

Choose a reason for hiding this comment

calebcordry Oct 2, 2020

Choose a reason for hiding this comment

calebcordry commented Sep 28, 2020

google-cla bot commented Sep 30, 2020

lgtm-com bot commented Sep 30, 2020

google-cla bot commented Oct 2, 2020

glendza1 commented Oct 2, 2020

lgtm-com bot commented Oct 2, 2020

googlebot commented Oct 2, 2020

calebcordry commented Oct 2, 2020

amp-owners-bot bot commented Aug 28, 2020 •

edited by ampprojectbot

calebcordry commented Aug 28, 2020 •

edited