feat: Add audioPosterMode option #7629

harisha-swaminathan · 2022-02-01T15:13:02Z

Description

Adds audioPosterMode option that is false by default. Turning the audioPosterMode on will enable the Poster Viewer experience by hiding the video element and persistently displaying the poster image. If no poster is defined, the video player will be black.

welcome · 2022-02-01T15:13:04Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

misteroneill

Looks great, overall! Some thoughts and suggestions follow...

src/js/player.js

test/unit/player.test.js

src/css/components/_poster.scss

…logic

mister-ben · 2022-02-04T14:11:27Z

Either as part of this PR or as a followup, we should consider implementing userAction.click and doubleClick on the poster instead of the tech in audio poster mode - #7577

misteroneill · 2022-02-08T13:58:54Z

@mister-ben Yeah, we should address that, but let's do it in a separate PR.

codecov · 2022-02-08T14:01:34Z

Codecov Report

Merging #7629 (38c8a6a) into main (eeda26f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7629      +/-   ##
==========================================
+ Coverage   80.24%   80.27%   +0.03%     
==========================================
  Files         116      116              
  Lines        7325     7337      +12     
  Branches     1771     1774       +3     
==========================================
+ Hits         5878     5890      +12     
  Misses       1447     1447

Impacted Files	Coverage Δ
src/js/player.js	`88.03% <100.00%> (+0.09%)`	⬆️

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 eeda26f...38c8a6a. Read the comment docs.

mister-ben · 2022-02-08T15:43:36Z

We should have some documentation in docs/guides/options.md until docs are moved elsewhere, otherwise looks good.

gkatsev

Works as expected and the implementation is pretty simple which is great 👍🏻

gkatsev · 2022-02-08T17:22:30Z

src/js/player.js

+    this.options_.audioPosterMode = value;
+
+    if (this.options_.audioPosterMode) {
+      this.tech_.hide();


thought: hiding the tech explicitly could potentially cause playback issues, particularly with other techs like youtube. Though, I'm not positive. Probably not worth handling now, but might need another solution if we do run into such a problem in the future.

gkatsev · 2022-02-08T17:27:43Z

src/js/player.js

+    value = value && Boolean(value);
+
+    if (value === undefined || value === this.options_.audioPosterMode) {
+      return this.options_.audioPosterMode;
+    }


if value is falsey, this will maintain value as that value, for example "" || Boolean("") will return "" rather than false.

Might be better to do a type check below. Something like

Suggested change

value = value && Boolean(value);

if (value === undefined || value === this.options_.audioPosterMode) {

return this.options_.audioPosterMode;

}

value = value && Boolean(value);

if (typeof value === 'undefined' || typeof value !== "boolean" || value === this.options_.audioPosterMode) {

return this.options_.audioPosterMode;

}

alex-barstow · 2022-02-10T16:10:44Z

src/js/player.js

+      return this.options_.audioPosterMode;
+    }
+
+    this.options_.audioPosterMode = value;


Rather than storing and modifying the audioPosterMode state on the options_ object itself, it might be preferable to create a new flag this.audioPosterMode_. That way this.options_.audioPosterMode always returns the option value set at initialization, and this.audioPosterMode_ returns the current state

misteroneill · 2022-02-10T21:20:31Z

src/js/player.js

+    value = value && Boolean(value);
+
+    if (value === undefined || typeof value !== 'boolean' || value === this.audioPosterMode_) {


I think given this if statement, we don't need to cast to boolean at all, right? And we shouldn't need the value === undefined condition either, right?

welcome · 2022-03-01T20:50:49Z

Congrats on merging your first pull request! 🎉🎉🎉

feat: Add audioPosterMode option

224e650

misteroneill requested changes Feb 1, 2022

View reviewed changes

src/js/player.js Outdated Show resolved Hide resolved

src/js/player.js Outdated Show resolved Hide resolved

test/unit/player.test.js Outdated Show resolved Hide resolved

src/css/components/_poster.scss Show resolved Hide resolved

separate tests into multiple tests and modify audioPosterMode getter …

6ae5d02

…logic

combine the vjs-poster css blocks

4d25db3

harisha-swaminathan requested a review from misteroneill February 5, 2022 00:01

misteroneill approved these changes Feb 8, 2022

View reviewed changes

misteroneill added the needs: LGTM Needs one or more additional approvals label Feb 8, 2022

gkatsev reviewed Feb 8, 2022

View reviewed changes

alex-barstow reviewed Feb 10, 2022

View reviewed changes

add type check, audiopostermode_ flag and update options doc

428847e

alex-barstow approved these changes Feb 10, 2022

View reviewed changes

misteroneill reviewed Feb 10, 2022

View reviewed changes

remove redundant type casting and if condition

38c8a6a

misteroneill approved these changes Feb 10, 2022

View reviewed changes

misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Feb 14, 2022

misteroneill added the minor This PR can be added to a minor release. It should not be added to a patch release. label Feb 22, 2022

misteroneill mentioned this pull request Feb 22, 2022

feat: Audio Only Mode #7647

Merged

misteroneill merged commit 64e55f5 into videojs:main Mar 1, 2022

misteroneill mentioned this pull request Apr 5, 2022

Sync guides with changes made in the main video.js repository since moving them here videojs/videojs.com#134

Merged

edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023

feat: Add audioPosterMode option (videojs#7629)

3fd8714

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add audioPosterMode option #7629

feat: Add audioPosterMode option #7629

harisha-swaminathan commented Feb 1, 2022

welcome bot commented Feb 1, 2022

misteroneill left a comment

mister-ben commented Feb 4, 2022

misteroneill commented Feb 8, 2022

codecov bot commented Feb 8, 2022 •

edited

Loading

mister-ben commented Feb 8, 2022

gkatsev left a comment

gkatsev Feb 8, 2022

gkatsev Feb 8, 2022

alex-barstow Feb 10, 2022

misteroneill Feb 10, 2022

welcome bot commented Mar 1, 2022

		value = value && Boolean(value);

		if (value === undefined \|\| typeof value !== 'boolean' \|\| value === this.audioPosterMode_) {

feat: Add audioPosterMode option #7629

feat: Add audioPosterMode option #7629

Conversation

harisha-swaminathan commented Feb 1, 2022

Description

welcome bot commented Feb 1, 2022

misteroneill left a comment

Choose a reason for hiding this comment

mister-ben commented Feb 4, 2022

misteroneill commented Feb 8, 2022

codecov bot commented Feb 8, 2022 • edited Loading

Codecov Report

mister-ben commented Feb 8, 2022

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev Feb 8, 2022

Choose a reason for hiding this comment

gkatsev Feb 8, 2022

Choose a reason for hiding this comment

alex-barstow Feb 10, 2022

Choose a reason for hiding this comment

misteroneill Feb 10, 2022

Choose a reason for hiding this comment

welcome bot commented Mar 1, 2022

codecov bot commented Feb 8, 2022 •

edited

Loading