Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(Video): fix muted prop#1847

Merged
layershifter merged 2 commits intomasterfrom
fix/video-mute
Aug 26, 2019
Merged

fix(Video): fix muted prop#1847
layershifter merged 2 commits intomasterfrom
fix/video-mute

Conversation

@layershifter
Copy link
Member

@layershifter layershifter commented Aug 26, 2019 •
edited
Loading

This PR fixes the muted prop on Video component, now it works properly.

Tested in Chrome, FF, Edge and IE11.

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Aug 26, 2019
layershifter
layershifter commented Aug 26, 2019
View reviewed changes
packages/react/src/components/Video/Video.tsx
} else {
this.videoRef.current.removeAttribute('muted')
}
this.videoRef.current.muted = !!this.props.muted
Copy link
Member Author

@layershifter layershifter Aug 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

facebook/react#10389 (comment)

We should set muted directly, setAttribute only shows this attribute on DOM.

@layershifter layershifter marked this pull request as ready for review August 26, 2019 09:51
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   69.64%   69.64%   +<.01%     
==========================================
  Files         874      874              
  Lines        7612     7610       -2     
  Branches     2216     2215       -1     
==========================================
- Hits         5301     5300       -1     
+ Misses       2303     2302       -1     
  Partials        8        8
Impacted Files Coverage Δ
packages/react/src/components/Video/Video.tsx 81.81% <100%> (+4.89%) ⬆️

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 0d7c53e...5e0f226. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Aug 26, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   69.64%   69.64%   +<.01%     
==========================================
  Files         874      874              
  Lines        7612     7610       -2     
  Branches     2216     2215       -1     
==========================================
- Hits         5301     5300       -1     
+ Misses       2303     2302       -1     
  Partials        8        8
Impacted Files Coverage Δ
packages/react/src/components/Video/Video.tsx 81.81% <100%> (+4.89%) ⬆️

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 0d7c53e...5e0f226. Read the comment docs.

@layershifter layershifter merged commit 3971070 into master Aug 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/video-mute branch August 26, 2019 12:02
@layershifter layershifter mentioned this pull request Sep 13, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@kuzhelov kuzhelov Awaiting requested review from kuzhelov

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny miroslavstastny is a code owner

@mnajdova mnajdova Awaiting requested review from mnajdova

1 more reviewer

@lucivpav lucivpav lucivpav approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🧰 fix Introduces fix for broken behavior. 🚀 ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@layershifter @lucivpav