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

Disallow amp-youtube in amp4ads #24864

Merged
merged 2 commits into from Oct 3, 2019
Merged

Disallow amp-youtube in amp4ads #24864

merged 2 commits into from Oct 3, 2019

Conversation

powerivq
Copy link
Contributor

@powerivq powerivq commented Oct 2, 2019

I2I sent in March, deprecation notice sent in July

#21340

@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 2, 2019

Hey @ampproject/wg-caching, these files were changed:

  • extensions/amp-youtube/validator-amp-youtube.protoascii

@powerivq powerivq mentioned this pull request Oct 2, 2019
Merged
@Gregable
Copy link
Member

Gregable commented Oct 2, 2019

@lannka

@honeybadgerdontcare
Copy link
Contributor

Please also update extensions.html and run gulp validator --update-tests to update the corresponding .out file.

@honeybadgerdontcare honeybadgerdontcare mentioned this pull request Oct 2, 2019
Closed
@honeybadgerdontcare
Copy link
Contributor

And also update validator_test.js

lannka
lannka approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

No amp-youtube usage in ads has been seen recently.

Pls also remove this error message in a separate PR:

amphtml/extensions/amp-youtube/0.1/amp-youtube.js

Line 153 in 8343281

'amp-youtube is deprecated in AMPHTML ads. See https://github.com/ampproject/amphtml/issues/21340'

@powerivq
Copy link
Contributor Author

powerivq commented Oct 2, 2019

Test updated.

honeybadgerdontcare
honeybadgerdontcare approved these changes Oct 2, 2019
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.

validation changes look good

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #24864 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24864      +/-   ##
==========================================
+ Coverage   79.74%   79.74%   +<.01%     
==========================================
  Files         852      852              
  Lines       51755    51761       +6     
==========================================
+ Hits        41271    41276       +5     
- Misses      10484    10485       +1
Flag Coverage Δ
#integration_tests 31.45% <ø> (+0.01%) ⬆️
#unit_tests 79.18% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...nsions/amp-analytics/0.1/vendors/tealiumcollect.js 100% <ø> (ø) ⬆️
extensions/amp-analytics/0.1/config.js 86.48% <100%> (+0.45%) ⬆️
extensions/amp-brightcove/0.1/amp-brightcove.js 72.72% <0%> (-0.76%) ⬇️
extensions/amp-date-picker/0.1/amp-date-picker.js 84.16% <0%> (-0.5%) ⬇️
extensions/amp-carousel/0.1/slidescroll.js 90.54% <0%> (+0.36%) ⬆️
src/curve.js 82.05% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82d2818...4699cbc. Read the comment docs.

@rcebulko rcebulko self-requested a review October 3, 2019 15:14
@powerivq powerivq merged commit 46b05c1 into ampproject:master Oct 3, 2019
@powerivq powerivq deleted the amp-youtube-amp4ads branch October 3, 2019 17:34
banaag added a commit to banaag/amphtml that referenced this pull request Oct 9, 2019
@banaag
cl/272744584 Revision bump for ampproject#24864
3816173
@banaag banaag mentioned this pull request Oct 9, 2019
Merged
banaag added a commit that referenced this pull request Oct 9, 2019
@banaag
Validator rollup (#24986)
9d63841 
* cl/272580305 Fix small performance bug to use result of shouldRecordTagspecValidated

 Author:    Greg Grothaus <greggrothaus@google.com>
 Committer: Allan Banaag <banaag@google.com>

 Changes to be committed:
	modified:   validator/engine/validator.js

* cl/272744584 Revision bump for #24864
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gregable Gregable Gregable approved these changes

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@lannka lannka lannka approved these changes

@andrewwatterson andrewwatterson Awaiting requested review from andrewwatterson

@banaag banaag Awaiting requested review from banaag

@rcebulko rcebulko Awaiting requested review from rcebulko

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@powerivq @Gregable @honeybadgerdontcare @lannka @googlebot