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

fix(viewer): Pause audio instead of muting it if autoplay is prevented #1068

Merged
merged 9 commits into from
Sep 17, 2019
Merged

fix(viewer): Pause audio instead of muting it if autoplay is prevented #1068

merged 9 commits into from
Sep 17, 2019

Conversation

mxiao6
Copy link
Contributor

@mxiao6 mxiao6 commented Sep 10, 2019

No description provided.

@mxiao6 mxiao6 requested a review from a team as a code owner September 10, 2019 00:41
@boxcla
Copy link

boxcla commented Sep 10, 2019

Verified that @mxiao6 has signed the CLA. Thanks for the pull request!

@mxiao6 mxiao6 changed the title fix(viewer): Audio cannot autoplay in Chrome fix(viewer): Pause audio instead of muting it if autoplay is prevented in browsers Sep 11, 2019
@mxiao6 mxiao6 changed the title fix(viewer): Pause audio instead of muting it if autoplay is prevented in browsers fix(viewer): Pause audio instead of muting it if autoplay is prevented Sep 11, 2019
ConradJChan
ConradJChan reviewed Sep 11, 2019
View reviewed changes
Copy link
Contributor

@ConradJChan ConradJChan left a comment

Choose a reason for hiding this comment

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

Also, would it make sense to have autoplay be a noop in MediaBaseViewer and have the autoplay implementations live in MP3Viewer and VideoBaseViewer?

src/lib/viewers/media/MP3Viewer.js Outdated Show resolved Hide resolved
src/lib/viewers/media/MP3Viewer.js Outdated Show resolved Hide resolved
src/lib/viewers/media/MediaBaseViewer.js Outdated Show resolved Hide resolved
jstoffan
jstoffan reviewed Sep 12, 2019
View reviewed changes
Copy link
Collaborator

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

Change looks good to me, just a few questions and some typos to fix.

src/lib/viewers/media/MediaBaseViewer.js Show resolved Hide resolved
src/lib/viewers/media/MediaBaseViewer.js Outdated Show resolved Hide resolved
src/lib/viewers/media/MediaBaseViewer.js Outdated Show resolved Hide resolved
ConradJChan
ConradJChan reviewed Sep 17, 2019
View reviewed changes
src/lib/viewers/media/MediaBaseViewer.js Show resolved Hide resolved
src/lib/viewers/media/MediaBaseViewer.js Outdated Show resolved Hide resolved
src/lib/viewers/media/MediaBaseViewer.js Outdated Show resolved Hide resolved
@mergify mergify bot merged commit 09b22cf into box:master Sep 17, 2019
@mxiao6 mxiao6 deleted the 6883-autoplay-chrome branch December 11, 2019 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstoffan jstoffan jstoffan approved these changes

@ConradJChan ConradJChan ConradJChan approved these changes

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mxiao6 @boxcla @jstoffan @ConradJChan