-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
feat: Audio Only Mode #7647
feat: Audio Only Mode #7647
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7647 +/- ##
==========================================
+ Coverage 80.64% 80.74% +0.10%
==========================================
Files 116 116
Lines 7373 7418 +45
Branches 1782 1794 +12
==========================================
+ Hits 5946 5990 +44
- Misses 1427 1428 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Miscellaneous thoughts!
Co-authored-by: Pat O'Neill <pgoneill@gmail.com>
Co-authored-by: Pat O'Neill <pgoneill@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, change of plans!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of tiny comments
src/js/player.js
Outdated
// Fullscreen is not supported in audioOnlyMode, so exit if we need to | ||
if (this.isFullscreen()) { | ||
this.exitFullscreen(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also exit PiP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we exit fs/pip before getting the control bar height, in case it's different in fullscreen? At least exitFullscreen should have a promise for knowing when exiting has finished.
also, if you set |
Co-authored-by: Gary Katsevman <git@gkatsev.com>
Co-authored-by: Gary Katsevman <git@gkatsev.com>
@@ -2161,6 +2162,7 @@ QUnit.test('When VIDEOJS_NO_DYNAMIC_STYLE is set, apply sizing directly to the t | |||
assert.equal(player.tech_.el().width, 600, 'the width is equal to 600'); | |||
assert.equal(player.tech_.el().height, 300, 'the height is equal 300'); | |||
player.dispose(); | |||
window.VIDEOJS_NO_DYNAMIC_STYLE = originalVjsNoDynamicStyling; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This state was leaking, causing the below 'audioOnlyMode(true/false) changes player height'
test to fail.
Really cool feature @alex-barstow. I was surprised to see it in a >= 7.18.1 release, I would've preferred to see it in a major release.. Fortunately it didn't break anything in my 'audio only plugin' for video.js which I've been maintaining since video.js 4.x: https://github.com/collab-project/videojs-wavesurfer Now that video.js finally supports an audio-only mode, I guess I'll have to revisit my plugin at some point and remove all the nasty hacks in order to support wavesurfer.js/audio-only. ps. because fairly major things are still added to video.js 7.x, can we expect the 7.x versioning to stay for(ever/awhile) or will there be a video.js 8 at some point @gkatsev? |
Video.js 8 is coming. Since this is a new feature, with theoretically no breakages, it should come in a minor release (it was released in 7.19.0). Would love to see an updated version of your plugin that interfaces with the audio-only mode. |
* audioOnlyMode wip * fix incorrect logs * add tests * minor code changes and add another test * update docs * fix formatting * fix typo * Consolidate conditions Co-authored-by: Pat O'Neill <pgoneill@gmail.com> * Compare objects instead of name string Co-authored-by: Pat O'Neill <pgoneill@gmail.com> * code review changes * remove unnecessary equivalence check Co-authored-by: Gary Katsevman <git@gkatsev.com> * replace height() with currentHeight() Co-authored-by: Gary Katsevman <git@gkatsev.com> * rewrite for async pip and fs handling * asyncify tests * update doc * add test Co-authored-by: Pat O'Neill <pgoneill@gmail.com> Co-authored-by: Gary Katsevman <git@gkatsev.com>
Description
This adds support for
audioOnlyMode
, which will hide all player children except the control bar, and within the control bar any controls that are only needed for video.This can be enabled vis either the new
player.audioOnlyMode()
getter/setter or aaudioOnlyMode
option, which defaults tofalse
Specific Changes proposed
When
audioOnlyMode
is enabled:CaptionsButton
,DescriptionButton
,SubsCapsButton
,FullscreenToggle
,PictureInPictureToggle
(this is done with CSS rather than JS to avoid unneeded complexity since some of these controls show/hide themselves dynamically depending on available support and/or the presence of certain text tracks.PlaybackRateMenuButton
,ChaptersButton
)'audioonlymodechange'
event