-
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
Add userAction.click to prevent pause/play when player is clicked #7495
Conversation
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
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. |
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.
Thanks!
It's definitely been a well requested feature. Making it a user action makes total sense.
Are you able to add some tests? Should be in here https://github.com/videojs/video.js/blob/main/test/unit/player-user-actions.test.js#L8
Also, can you revert the package-lock.json changes?
@@ -443,6 +444,40 @@ Defines the order in which Video.js techs are preferred. By default, this means | |||
|
|||
> Type: `Object` | |||
|
|||
### `userActions.click` |
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.
Thanks for updating the documentation!
src/js/player.js
Outdated
this.options_ === undefined || | ||
this.options_.userActions === undefined || | ||
this.options_.userActions.click === undefined || | ||
this.options_.userActions.click !== false |
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.
nit: spacing
this.options_ === undefined || | |
this.options_.userActions === undefined || | |
this.options_.userActions.click === undefined || | |
this.options_.userActions.click !== false | |
this.options_ === undefined || | |
this.options_.userActions === undefined || | |
this.options_.userActions.click === undefined || | |
this.options_.userActions.click !== false |
src/js/player.js
Outdated
this.options_ !== undefined && | ||
this.options_.userActions !== undefined && | ||
typeof this.options_.userActions.click === 'function' |
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.
nit: spacing
this.options_ !== undefined && | |
this.options_.userActions !== undefined && | |
typeof this.options_.userActions.click === 'function' | |
this.options_ !== undefined && | |
this.options_.userActions !== undefined && | |
typeof this.options_.userActions.click === 'function' |
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.
Yeah, I rarely edit javascript and recently switched to doom emacs and don't have the auto indention working right yet.
Fixed the Spacing and reverted the package-lock.json.
I spent a couple hours trying to write the unit tests and it wasn't working out. Like I said, I don't normally write javascript and am not familiar with the test framework. And unfortunately this was already some deep Yak shaving by the Bike Shed at the end of a big detour trying to get contorted aspect ratios and css layout working on my main task that I'm already weeks behind on.
Maybe there is someone lurking who is familiar with the test framework could add that? Conceptually it should be a trivial variation on the userAction.doubeClick tests. But I was not having any luck with that.
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.
I'll see if I can get a few minutes to write a test or two tomorrow.
Codecov Report
@@ Coverage Diff @@
## main #7495 +/- ##
==========================================
- Coverage 79.72% 79.61% -0.11%
==========================================
Files 116 116
Lines 7296 7306 +10
Branches 1754 1763 +9
==========================================
Hits 5817 5817
- Misses 1479 1489 +10
Continue to review full report at Codecov.
|
Congrats on merging your first pull request! 🎉🎉🎉 |
…ed (videojs#7495) Pass `false` as `userAction.click` to disable the default click-to-play behavior. Alternatively, pass in a function, to enable custom behavior. Fixes videojs#7123.
Description
Over the ages, many people have requested the ability to make it so clicking on the player does NOT toggle pause/play but all the other nice features such as still controlling the video via the control-bar and having the controls fade in and out based on pointer actions still work.
The most recent issue was
where @gkatsev showed what was needed to be done and requested a PR.
This is a proposed PR to fix and possibly:
The
pointer-events: none;
CSS suggestion does not do the same as it disables the show/hide of the control bar and other impacts.Specific Changes proposed
Add a userAction for the single click event to override the pause/play action.
Requirements Checklist
/sandbox/userAction-click.html.example