Inline volumeMenuButton option #2378

mmcc · 2015-07-18T01:43:33Z

This adds a new inline option to MenuButton that allows you to use MenuButton as a drawer of sorts. Old functionality is unchanged, although the styles now reside under .vjs-menu-button-popup.

To use, simply pass inline: true in the volumeMenuButton options. This will also default to a horizontal slider for obvious reasons.

videojs('vid1', { "playbackRates": [1, 2, 3], "controlBar": { "volumeMenuButton": { "inline": true }}});

There's an unrelated issue in IE8 where bubbling seems to be messed up. Everything works as far as this PR is concerned, but the bubbling issue is causing any click on the menu to count for a click on the mute button. We need to sort that out still, but I don't think this PR is the place for it.

pam · 2015-07-18T01:49:23Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 1b28a9329a1cac2915aa84c7d7d3e6e1fd736045
Build details: https://travis-ci.org/pam/video.js/builds/71510224

(Please note that this is a fully automated comment.)

heff · 2015-07-18T18:40:13Z

Very cool!

How do I test it? I tried:

"controlBar": { "volumeMenuButton": { "inline": true } }

and got

It'd be nice if we could just do volumeMenuButton": { "inline": true } at the top level of the player options. Now that we're passing playerOptions to children we could probably do that easily. Don't try to add that to this PR though.

mmcc · 2015-07-19T02:31:01Z

@heff you also need to specify vertical: false.

"controlBar": { "volumeMenuButton": {"vertical": "false, "inline": true } }

gkatsev · 2015-07-19T03:06:30Z

Can inline: true imply vertical? I don't think there would be any case where you would want it to be inline and also vertical. And if you did, you could always set vertical to true to force it.

mmcc · 2015-07-20T16:24:40Z

inline: true can imply vertical for the volumeMenuButton specifically, but keep in mind that the option is intended to be generic to any instance of MenuButton.

gkatsev · 2015-07-20T16:28:04Z

That's true. But the volumeMenuButton can default to vertical: false when inline: true. I'm not certain that we do want this, but it does seem to simplify the simple base case.

pam · 2015-07-20T17:08:06Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 4b6bcdf1935f071731d8133bf40c24d30c4cf9cc
Build details: https://travis-ci.org/pam/video.js/builds/71802376

(Please note that this is a fully automated comment.)

mmcc · 2015-07-20T17:35:30Z

@gkatsev @heff I went ahead and made the switch to set vertical: false if inline: true is specified on an instance of VolumeMenuButton. Tests are currently failing because of the node-sass issue on Travis.

heff · 2015-07-20T17:41:24Z

src/js/control-bar/volume-menu-button.js

@@ -19,6 +19,12 @@ import VolumeBar from './volume-control/volume-bar.js';
 class VolumeMenuButton extends MenuButton {

  constructor(player, options={}){
+    // If an inline volumeMenuButton is used, we should default to using a horizontal
+    // slider for obvious reasons.
+    if (options.inline) {


Might as well move this inside the options.vertical === undefined check, so it's overridable.

heff · 2015-07-20T17:46:53Z

A few more comments, not blockers. LGTM

pam · 2015-07-20T18:17:16Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 4e68e6fe64d1b313053ce3f13458f8d1c7bccbf0
Build details: https://travis-ci.org/pam/video.js/builds/71813262

(Please note that this is a fully automated comment.)

pam · 2015-07-21T23:05:50Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: ad810aeddd9f3729007be3c6932a0f2086b1e29f
Build details: https://travis-ci.org/pam/video.js/builds/72034158

(Please note that this is a fully automated comment.)

pam · 2015-07-22T00:00:21Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 34a38c3a6e8f6d9350e56e6ce433d2b5ea7517ed
Build details: https://travis-ci.org/pam/video.js/builds/72039643

(Please note that this is a fully automated comment.)

pam · 2015-07-22T00:34:37Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 70e0c4877ea40cb9d824a167d219a7bee0d6a681
Build details: https://travis-ci.org/pam/video.js/builds/72042593

(Please note that this is a fully automated comment.)

mmcc · 2015-07-24T20:27:23Z

Bumping this thread, I think this is good to pull in. @dmlap @gkatsev - thoughts?

mmcc · 2015-07-24T20:30:11Z

Also to note, I've personally tested this in:

OSX: Chrome, Firefox, Safari
Windows: Chrome, Firefox, IE8, IE11

pam · 2015-07-24T20:31:17Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: a2c56a71107279168465facc27ec0654d3c43adb
Build details: https://travis-ci.org/pam/video.js/builds/72522493

(Please note that this is a fully automated comment.)

pam · 2015-07-24T20:55:11Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 7a842e2
Build details: https://travis-ci.org/pam/video.js/builds/72525471

(Please note that this is a fully automated comment.)

heff · 2015-07-25T00:45:48Z

Nice, you ere able to fix IE8? LGTM

mmcc · 2015-07-28T23:25:02Z

@heff Everything related to this PR is working in IE8, but we still need to loop back on the bubbling issue.

heff · 2015-07-29T17:12:24Z

@mmcc can you create a new issue for the bubbling?

Sent from mobile

heff reviewed Jul 20, 2015
View reviewed changes

mmcc force-pushed the inline-volume-expansion branch from 4e68e6f to ad810ae Compare July 21, 2015 22:59

mmcc changed the title ~~Initial stab at inline volume~~ Inline volume Jul 24, 2015

mmcc changed the title ~~Inline volume~~ Inline volumeMenuButton Jul 24, 2015

mmcc changed the title ~~Inline volumeMenuButton~~ Inline volumeMenuButton option Jul 24, 2015

mmcc added 8 commits July 24, 2015 13:48

initial stab at making this work

d5b5b90

default to vertical: false for an inline volumeMenuButton

bc9e231

move options.inline check into vertical undefined check

3c63076

add comment about volume-specific style in inline

14696d6

volume slider and progress bar now align properly

d7b6c15

play pause is just like the rest of us

9af38b5

hide the scrollbars

0c27d29

fixed issues in IE8

7914716

fixed positioning styles now that handle is smaller

7a842e2

mmcc force-pushed the inline-volume-expansion branch from a2c56a7 to 7a842e2 Compare July 24, 2015 20:48

mmcc closed this in e330e7b Jul 28, 2015

mmcc mentioned this pull request Jul 29, 2015

Bubbling Issues in IE8 #2414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline volumeMenuButton option #2378

Inline volumeMenuButton option #2378

mmcc commented Jul 18, 2015

pam commented Jul 18, 2015

heff commented Jul 18, 2015

mmcc commented Jul 19, 2015

gkatsev commented Jul 19, 2015

mmcc commented Jul 20, 2015

gkatsev commented Jul 20, 2015

pam commented Jul 20, 2015

mmcc commented Jul 20, 2015

heff Jul 20, 2015

heff commented Jul 20, 2015

pam commented Jul 20, 2015

pam commented Jul 21, 2015

pam commented Jul 22, 2015

pam commented Jul 22, 2015

mmcc commented Jul 24, 2015

mmcc commented Jul 24, 2015

pam commented Jul 24, 2015

pam commented Jul 24, 2015

heff commented Jul 25, 2015

mmcc commented Jul 28, 2015

heff commented Jul 29, 2015

Inline volumeMenuButton option #2378

Inline volumeMenuButton option #2378

Conversation

mmcc commented Jul 18, 2015

pam commented Jul 18, 2015

heff commented Jul 18, 2015

mmcc commented Jul 19, 2015

gkatsev commented Jul 19, 2015

mmcc commented Jul 20, 2015

gkatsev commented Jul 20, 2015

pam commented Jul 20, 2015

mmcc commented Jul 20, 2015

heff Jul 20, 2015

Choose a reason for hiding this comment

heff commented Jul 20, 2015

pam commented Jul 20, 2015

pam commented Jul 21, 2015

pam commented Jul 22, 2015

pam commented Jul 22, 2015

mmcc commented Jul 24, 2015

mmcc commented Jul 24, 2015

pam commented Jul 24, 2015

pam commented Jul 24, 2015

heff commented Jul 25, 2015

mmcc commented Jul 28, 2015

heff commented Jul 29, 2015