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

Jump to bottom

✨ Remixd AMP ad-tag implementation #30030 #30572

Merged
merged 4 commits into from
Oct 15, 2020
Merged

✨ Remixd AMP ad-tag implementation #30030 #30572

merged 4 commits into from
Oct 15, 2020

Conversation

glendza1
Copy link
Contributor

@glendza1 glendza1 commented Oct 8, 2020

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

@google-cla google-cla bot added the cla: yes label Oct 8, 2020
@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 8, 2020 •
edited by ampprojectbot

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

validator/testdata/feature_tests/ads.html

@lgtm-com
Copy link

lgtm-com bot commented Oct 8, 2020

This pull request introduces 1 alert when merging 55540bf into dd32d79 - view on LGTM.com

new alerts:

  • 1 for Call to eval-like DOM function

honeybadgerdontcare
honeybadgerdontcare approved these changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Reviewing validator related changes only, they look good.

@MichaelRybak MichaelRybak removed their request for review October 8, 2020 17:18
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@calebcordry
Copy link
Member

Hey @glendza looks like the CLA worked on this one. I think you will still need to make the new validator out files to get the CI to pass. Also a few lint errors still gulp lint thanks for keeping at this.

@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2020

This pull request introduces 1 alert when merging a4461f6 into b025f30 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

This pull request introduces 1 alert when merging 7c28f2a into bf88a60 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

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

new alerts:

  • 1 for Call to eval-like DOM function

calebcordry
calebcordry approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@calebcordry calebcordry left a comment

Choose a reason for hiding this comment

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

Thanks for your patience with this one!

@calebcordry calebcordry merged commit 5fa3930 into ampproject:master Oct 15, 2020
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@glendza1 @ed-bird
✨ 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@calebcordry calebcordry calebcordry approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental WG: caching
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@glendza1 @CLAassistant @calebcordry @honeybadgerdontcare @Gregable @ampprojectbot