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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃彈 Ensure that list on amp-video-interface.md is complete #32810

Merged
merged 26 commits into from Feb 23, 2021
Merged

馃彈 Ensure that list on amp-video-interface.md is complete #32810

merged 26 commits into from Feb 23, 2021

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Feb 22, 2021 •
edited

Successful --fix run:

gulp check-video-interface-list --fix

[09:21:06] Using gulpfile ~/git/amphtml/gulpfile.js
[09:21:06] Starting 'check-video-interface-list'...

diff --git a/spec/amp-video-interface.md b/spec/amp-video-interface.md
index b67fb4364..a59bf9ec0 100644
--- a/spec/amp-video-interface.md
+++ b/spec/amp-video-interface.md
@@ -30,2 +30,3 @@ these players implement.
 -   [amp-ima-video](https://amp.dev/documentation/components/amp-ima-video)
+-   [amp-jwplayer](https://amp.dev/documentation/components/amp-jwplayer)
 -   [amp-minute-media-player](https://amp.dev/documentation/components/amp-minute-media-player)
@@ -35,2 +36,3 @@ these players implement.
 -   [amp-powr-player](https://amp.dev/documentation/components/amp-powr-player)
+-   [amp-redbull-player](https://amp.dev/documentation/components/amp-redbull-player)
 -   [amp-vimeo](https://amp.dev/documentation/components/amp-vimeo)

[09:21:07] Wrote spec/amp-video-interface.md
[09:21:07] Finished 'check-video-interface-list' after 620 ms

A check failure run is the same, except it errors-out when the list is incomplete.

@amp-owners-bot
Copy link

Hey @rsimha! These files were changed:

build-system/pr-check/checks.js

rsimha
rsimha reviewed Feb 22, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Nice PR! A few comments below.

build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
build-system/pr-check/checks.js Outdated Show resolved Hide resolved
build-system/pr-check/checks.js Outdated Show resolved Hide resolved
build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
build-system/tasks/video-interface-list.js Outdated Show resolved Hide resolved
rsimha
rsimha reviewed Feb 23, 2021
View reviewed changes
gulpfile.js Show resolved Hide resolved
rsimha
rsimha approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for patiently addressing all the feedback!

@alanorozco alanorozco removed the request for review from jridgewell February 23, 2021 18:27
rsimha
rsimha approved these changes Feb 23, 2021
View reviewed changes
spec/amp-video-interface.md Show resolved Hide resolved
@alanorozco alanorozco removed the request for review from erwinmombay February 23, 2021 22:33
@alanorozco alanorozco merged commit 9648728 into ampproject:master Feb 23, 2021
@alanorozco alanorozco deleted the video-list branch February 23, 2021 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

@rbeckthomas rbeckthomas Awaiting requested review from rbeckthomas

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@alanorozco @rsimha @ampprojectbot