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

Already on GitHub? Sign in to your account

馃悰Added Mute/Unmute Controls to <amp-ima-video> #18611

Merged
merged 3 commits into from Oct 9, 2018

Conversation

Projects
None yet
4 participants
@torch2424
Copy link
Member

torch2424 commented Oct 8, 2018

closes #18531

This adds mute/unmute controls to <amp-ima-video> 馃槃 Also, does some updating to the <amp-ima-video> that makes it usable on desktop, and offers more context on ima, and it's example implementation.

Examples

imavideomute

torch2424 added some commits Oct 6, 2018

@torch2424 torch2424 requested review from alanorozco and zhouyx Oct 8, 2018

@googlebot googlebot added the cla: yes label Oct 8, 2018

@torch2424

This comment has been minimized.

Copy link
Member Author

torch2424 commented Oct 8, 2018

Going to look for an IMA team member to give this a review 馃槃

@zhouyx

This comment has been minimized.

Copy link
Collaborator

zhouyx commented Oct 8, 2018

Should we put the mute button on the left side or right side?

@torch2424

This comment has been minimized.

Copy link
Member Author

torch2424 commented Oct 9, 2018

IMA team doesn't have bandwidth, and @spacedino approved of the media button on the right side 馃槃

cc @zhouyx

@zhouyx

This comment has been minimized.

Copy link
Collaborator

zhouyx commented Oct 9, 2018

One more request, can I see the UI in mobile? Otherwise looks awesome! Thank you for digging into the code and let's ship it!

@zhouyx

zhouyx approved these changes Oct 9, 2018

Copy link
Collaborator

zhouyx left a comment

LGTM!!

@torch2424

This comment has been minimized.

Copy link
Member Author

torch2424 commented Oct 9, 2018

Showed Mobile UI in person to @zhouyx

Thanks for the LGTM Merging 馃槃

@torch2424 torch2424 merged commit 094cb1d into ampproject:master Oct 9, 2018

4 checks passed

LGTM analysis: JavaScript No alert changes
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details

@torch2424 torch2424 deleted the torch2424:ima-video-mute-18531 branch Oct 9, 2018

noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Oct 18, 2018

馃悰Added Mute/Unmute Controls to <amp-ima-video> (ampproject#18611)
* Fixed the ima video example

* Finished the Mute/UnMute

* Fixed all testing and liniting errors

Enriqe added a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

馃悰Added Mute/Unmute Controls to <amp-ima-video> (ampproject#18611)
* Fixed the ima video example

* Finished the Mute/UnMute

* Fixed all testing and liniting errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.