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

📖 Guide to Building a Bento Video Player #34195

Merged
merged 19 commits into from
May 4, 2021

Conversation

alanorozco
Copy link
Member

No description provided.

@alanorozco alanorozco requested a review from rcebulko May 4, 2021 16:03
Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

This is great, super thorough! love the diffs as a way of explanation

contributing/building-a-bento-video-player.md Show resolved Hide resolved
allowlist: [
...
+ // Bento MyFantasticPlayer, amp-my-fantastic-player
+ 'extensions/amp-my-fantastic-player/1.0/base-element.js->extensions/amp-video/1.0/base-element.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth sectioning off the area of dep-check config that currently has YouTube and Vimeo to establish a video-player section. Also, will Bento components need a dep on the video-manager-impl.js service or is that only for AMP components?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer that, but I don't know if we should go against the convention in that file.

contributing/building-a-bento-video-player.md Outdated Show resolved Hide resolved
contributing/building-a-bento-video-player.md Show resolved Hide resolved
contributing/building-a-bento-video-player.md Outdated Show resolved Hide resolved
contributing/building-a-bento-video-player.md Show resolved Hide resolved
case 'pause':
dispatchCustomEvent(currentTarget, data.event, null, {
bubbles: true,
cancelable: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do bubbles/cancelable need to be wrapped in quotes? I have a hard time keeping track

Copy link
Member Author

Choose a reason for hiding this comment

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

Per type check, no, since we only want an Object.

This doesn't cause any issues since we don't do advanced compilation, and we cannot do it without singlepass. Does it even matter anymore?

contributing/building-a-bento-video-player.md Outdated Show resolved Hide resolved
contributing/building-a-bento-video-player.md Outdated Show resolved Hide resolved
contributing/building-a-bento-video-player.md Outdated Show resolved Hide resolved
@alanorozco alanorozco merged commit e22e0e1 into ampproject:main May 4, 2021
@alanorozco alanorozco deleted the docs-create-bento-video branch May 4, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants