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: allow progress controls to be disabled #4649

Merged
merged 6 commits into from Oct 31, 2017

Conversation

Projects
None yet
3 participants
@BrandonOCasey
Contributor

BrandonOCasey commented Oct 5, 2017

Description

Sometimes you do not want the user to be able to use the progress controls, such as during an ad. It would be quite difficult to do this outside of video.js due to the complexity of it. I think that we should allow this within video.js.

Specific Changes proposed

Add functions to disable/enable progress controls

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
@thijstriemstra

This comment has been minimized.

Show comment
Hide comment
@thijstriemstra

thijstriemstra Oct 13, 2017

Contributor

It would be quite difficult to do this outside of video.js due to the complexity of it.

Here's what I used:

if (this.player.options_.controls) {
    // progress control isn't used by this plugin
    this.player.controlBar.progressControl.hide();
}
Contributor

thijstriemstra commented Oct 13, 2017

It would be quite difficult to do this outside of video.js due to the complexity of it.

Here's what I used:

if (this.player.options_.controls) {
    // progress control isn't used by this plugin
    this.player.controlBar.progressControl.hide();
}
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 19, 2017

Member

Hm... I'm not sure we want this. Disabling built-ins like this seems really weird to me. Since I can see cases where you'd want the progress control but just disable seeking but right now this disables it completely, if I understand it correctly.
One thing that I was thinking off middleware is the ability to cancel a chain, this is something that's currently missing and I think might fit better for the specific usecase.

Member

gkatsev commented Oct 19, 2017

Hm... I'm not sure we want this. Disabling built-ins like this seems really weird to me. Since I can see cases where you'd want the progress control but just disable seeking but right now this disables it completely, if I understand it correctly.
One thing that I was thinking off middleware is the ability to cancel a chain, this is something that's currently missing and I think might fit better for the specific usecase.

@BrandonOCasey

This comment has been minimized.

Show comment
Hide comment
@BrandonOCasey

BrandonOCasey Oct 20, 2017

Contributor

Well the real issue here is @gkatsev there isn't a good way to disable dragging on the progress bar when you want to. Disabling clicks is easy enough. All that this PR does is disable interacting with the progress bar. It is still there and works as intended, you just cant use it to seek.

Contributor

BrandonOCasey commented Oct 20, 2017

Well the real issue here is @gkatsev there isn't a good way to disable dragging on the progress bar when you want to. Disabling clicks is easy enough. All that this PR does is disable interacting with the progress bar. It is still there and works as intended, you just cant use it to seek.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 24, 2017

Member

Yeah, maybe this isn't that bad. We should either keep listening to mousemove or we should hide the mouse time display when disabled. Also, we probably want to change the cursor to regular when the control bar is disabled.

Also, probably should just be called disable and enable.

Member

gkatsev commented Oct 24, 2017

Yeah, maybe this isn't that bad. We should either keep listening to mousemove or we should hide the mouse time display when disabled. Also, we probably want to change the cursor to regular when the control bar is disabled.

Also, probably should just be called disable and enable.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 24, 2017

Member

Maybe just hiding the mouse time display is best as another indicator that this is a "read-only" control would be better.

Member

gkatsev commented Oct 24, 2017

Maybe just hiding the mouse time display is best as another indicator that this is a "read-only" control would be better.

@BrandonOCasey

This comment has been minimized.

Show comment
Hide comment
@BrandonOCasey

BrandonOCasey Oct 24, 2017

Contributor

All good ideas, I will implement them when I get some time.

Contributor

BrandonOCasey commented Oct 24, 2017

All good ideas, I will implement them when I get some time.

@@ -8,6 +8,10 @@
min-width: 4em;
}
.video-js .vjs-progress-control.disabled {
cursor: default;

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 25, 2017

Contributor

we don't really want a pointer when its disabled

@BrandonOCasey

BrandonOCasey Oct 25, 2017

Contributor

we don't really want a pointer when its disabled

this.off(doc, 'mouseup', this.handleMouseUp);
this.off(doc, 'touchmove', this.handleMouseMove);
this.off(doc, 'touchend', this.handleMouseUp);
this.removeAttribute('tabindex');

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 25, 2017

Contributor

Don't allow tabbing to the slider when it is disabled

@BrandonOCasey

BrandonOCasey Oct 25, 2017

Contributor

Don't allow tabbing to the slider when it is disabled

This comment has been minimized.

@gkatsev

gkatsev Oct 25, 2017

Member

Good call.

@gkatsev

gkatsev Oct 25, 2017

Member

Good call.

Show outdated Hide outdated src/css/components/_progress.scss
Show outdated Hide outdated src/js/slider/slider.js
this.off(doc, 'mouseup', this.handleMouseUp);
this.off(doc, 'touchmove', this.handleMouseMove);
this.off(doc, 'touchend', this.handleMouseUp);
this.removeAttribute('tabindex');

This comment has been minimized.

@gkatsev

gkatsev Oct 25, 2017

Member

Good call.

@gkatsev

gkatsev Oct 25, 2017

Member

Good call.

BrandonOCasey and others added some commits Oct 26, 2017

@@ -40,6 +44,10 @@
font-size: 1.666666666666666666em;
}
.video-js .vjs-progress-control:hover .vjs-progress-holder.disabled {

This comment has been minimized.

@gkatsev

gkatsev Oct 26, 2017

Member

I don't think this block is necessary and it causes the time display not to look right
screen shot 2017-10-26 at 11 25 00
screen shot 2017-10-26 at 11 31 45
First is the "broken" one, second is the "correct" one.

Though, now that I think about it, this is probably on purpose so that the progress holder/seek handle isn't as large?

@gkatsev

gkatsev Oct 26, 2017

Member

I don't think this block is necessary and it causes the time display not to look right
screen shot 2017-10-26 at 11 25 00
screen shot 2017-10-26 at 11 31 45
First is the "broken" one, second is the "correct" one.

Though, now that I think about it, this is probably on purpose so that the progress holder/seek handle isn't as large?

@gkatsev

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM labels Oct 31, 2017

@gkatsev gkatsev merged commit a3c254e into master Oct 31, 2017

3 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gkatsev gkatsev deleted the feat/disable-progress-controls branch Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment