feat: Support toggling the select icon with the source label (#28) #56

PrestonN · 2020-02-17T23:17:27Z

User options can now be passed to the plugin. Options are passed upon initialization and use the following syntax:

require('@silvermine/videojs-quality-selector')(videojs, options);

If you're not using require, you can pass options using a window config:

<script>
   window.SILVERMINE_VIDEOJS_QUALITY_SELECTOR_CONFIG = {
      showQualitySelectionLabelInControlBar: true,
   };
</script>
<script src="path/to/silvermine-videojs-quality-selector.js"></script>

The options are sent from the constructor to /components/QualitySelector.js.

Currently only one option is available:

showQualitySelectionLabelInControlBar (default: false) - Enabling this flag will swap the selector icon with the label of the currently selected source.

More options should be possible by adding them to the object.

A class is added to the Menu Item (Item with class vjs-icon-placeholder is given the class vjs-quality-selector-icon) when the flag is disabled to include the icon. When enabled, basic HTML is simply included into the element.

Refs #28

…ine#28) User options can now be passed to the plugin. Options are passed upon initialization and use the following syntax: require('@silvermine/videojs-quality-selector')(videojs, options); If you're not using require, you can pass options using a window config: <script> window.SILVERMINE_VIDEOJS_QUALITY_SELECTOR_CONFIG = { showQualitySelectionLabelInControlBar: true, }; </script> <script src="path/to/silvermine-videojs-quality-selector.js"></script> The options are sent from the constructor to `/components/QualitySelector.js`. Currently only one option is available: showQualitySelectionLabelInControlBar: (default: `false`) - Enabling this flag will swap the selector icon with the label of the currently selected source. More options should be possible by adding them to the object. A class is added to the Menu Item (Has class vjs-icon-placeholder) when the flag is disabled to include the icon. When enabled, basic HTML is simply included into the element. Refs silvermine#28

jimjenkins5

LGTM. Thanks @PrestonN!

clwy-cn · 2020-04-22T19:23:26Z

@PrestonN! This is just what I need. Thanks

@jthomerson if this works fine, please merge it and npm publish

PrestonN · 2020-07-10T01:45:34Z

Hello!

Is there any update on when this will be merged? I'd love to see this work merged so that I can start using this functionality with my own project. If you want any changes to be made to it then please let me know. Hope to hear back from you. :)

syeopite · 2021-09-13T00:27:25Z

Hi,
It looks like this PR has been sitting for more than a year now. Is there any chance it could get merged soon?

Thanks!

Jazzman1976 · 2022-03-17T12:06:40Z

need this, too. Please merge it.

yokuze

@PrestonN Sorry for the delay. Thank you for the contribution. I added a few comments. When you address them, please don't use the "Commit suggestion" button since this creates separate commits for the fixes. Instead, please fix up the existing commits with the changes to keep the history clean.

This PR will also need a rebase against master since there are merge conflicts now.

yokuze · 2022-04-28T20:38:10Z

README.md

@@ -64,6 +64,35 @@ var videojs = require('videojs');
 require('@silvermine/videojs-quality-selector')(videojs);
 ```

+### Initialization options
+
+* **`showQualitySelectionLabelInControlBar`** (default: `false`) - Enabling this flag will swap the selector


Suggested change

* **`showQualitySelectionLabelInControlBar`** (default: `false`) - Enabling this flag will swap the selector

* **`showQualitySelectionLabelInControlBar`** (default: `false`) - When `true`, the

quality selector menu button will display the label of the currently selected source.

When `false`, the quality selector menu button will display an icon.

yokuze · 2022-04-28T20:39:32Z

src/js/components/QualitySelector.js

@@ -42,6 +45,15 @@ module.exports = function(videojs) {
         player.on(events.QUALITY_SELECTED, function(event, newSource) {
            // Update the selected source with the source that was actually selected
            this.setSelectedSource(newSource);
+
+            // Check for quality select label flag
+            if (userOptions.showQualitySelectionLabelInControlBar !== false) {


Suggested change

if (userOptions.showQualitySelectionLabelInControlBar !== false) {

if (userOptions.showQualitySelectionLabelInControlBar) {

yokuze · 2022-04-28T20:44:51Z

src/js/components/QualitySelector.js

+            } else if (qualitySelectMenuButton.className.indexOf('vjs-quality-selector-icon') === -1) {
+               // Add class to show gear icon.  Blank out innerHTML for safety precations.
+               qualitySelectMenuButton.className += ' vjs-quality-selector-icon';
+               qualitySelectMenuButton.innerHTML = '';


Why blank out innerHTML here? We lose the vjs-control-text element, which is important for accessibility.

yokuze · 2022-04-28T20:58:26Z

src/js/components/QualitySelector.js

+
+            // Check for quality select label flag
+            if (userOptions.showQualitySelectionLabelInControlBar !== false) {
+               qualitySelectMenuButton.innerHTML = newSource.label;


Setting innerHTML here wipes out the vjs-control-text element, which is important for accessibility. One way we can avoid doing that is to add an element to the button that displays the label text:

if (!this._qualitySelectorLabel) { this._qualitySelectorLabel = document.createElement('span'); this._qualitySelectorLabel.classList.add('vjs-quality-selector-label'); qualitySelectMenuButton.prepend(this._qualitySelectorLabel); } this._qualitySelectorLabel.innerHTML = newSource.label;

and then add this bit of CSS:

// Hide the quality selector icon if there is a quality selector label present .vjs-quality-selector-label ~ .vjs-quality-selector-icon { display: none; }

This would also make the else if section unnecessary.

yokuze · 2022-04-28T20:58:51Z

src/js/components/QualitySelector.js

@@ -42,6 +45,15 @@ module.exports = function(videojs) {
         player.on(events.QUALITY_SELECTED, function(event, newSource) {
            // Update the selected source with the source that was actually selected
            this.setSelectedSource(newSource);
+
+            // Check for quality select label flag


Suggested change

// Check for quality select label flag

// Check for quality selector label flag

PrestonN · 2022-05-01T22:07:49Z

Hi @yokuze, thanks for taking the time to look at this!

While working on the fixes that you've mentioned, I've come across 2 different problems while applying the changes.

For some reason qualitySelectMenuButton doesn't initialize properly any more unless MenuButton.call() is called beforehand. Due to your linting rules, it doesn't seem like this is desirable.
The CSS you provided doesn't seem to work as intended. I'll admit I'm not too familiar with the ~ handler, however looking it up it seems that since the new span element we're creating is a child to .vjs-quality-selector-icon so this won't get grabbed with your snippet. I can solve this issue using a little bit of JS to hide the icon, but I'm going to guess you'd prefer a pure CSS approach to this (outside of the JS that's currently there).

Please advice on how you'd like for me to approach these issues, and I will push my changes to this branch. Thanks again for taking the time to look at this. I'm really looking forward to getting this merged!

PrestonN force-pushed the master branch from aec1298 to b1a338f Compare February 18, 2020 00:44

jthomerson requested a review from jimjenkins5 April 5, 2020 23:10

jimjenkins5 approved these changes Apr 6, 2020

View reviewed changes

yokuze requested changes Apr 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support toggling the select icon with the source label (#28) #56

feat: Support toggling the select icon with the source label (#28) #56

PrestonN commented Feb 17, 2020

jimjenkins5 left a comment

clwy-cn commented Apr 22, 2020 •

edited

Loading

PrestonN commented Jul 10, 2020

syeopite commented Sep 13, 2021 •

edited

Loading

Jazzman1976 commented Mar 17, 2022

yokuze left a comment

yokuze Apr 28, 2022

yokuze Apr 28, 2022

yokuze Apr 28, 2022

yokuze Apr 28, 2022

yokuze Apr 28, 2022

PrestonN commented May 1, 2022

-* **`showQualitySelectionLabelInControlBar`** (default: `false`) - Enabling this flag will swap the selector
+* **`showQualitySelectionLabelInControlBar`** (default: `false`) - When `true`, the
+  quality selector menu button will display the label of the currently selected source.
+  When `false`, the quality selector menu button will display an icon.

	if (userOptions.showQualitySelectionLabelInControlBar !== false) {
	if (userOptions.showQualitySelectionLabelInControlBar) {

	// Check for quality select label flag
	// Check for quality selector label flag

feat: Support toggling the select icon with the source label (#28) #56

Are you sure you want to change the base?

feat: Support toggling the select icon with the source label (#28) #56

Conversation

PrestonN commented Feb 17, 2020

jimjenkins5 left a comment

Choose a reason for hiding this comment

clwy-cn commented Apr 22, 2020 • edited Loading

PrestonN commented Jul 10, 2020

syeopite commented Sep 13, 2021 • edited Loading

Jazzman1976 commented Mar 17, 2022

yokuze left a comment

Choose a reason for hiding this comment

yokuze Apr 28, 2022

Choose a reason for hiding this comment

yokuze Apr 28, 2022

Choose a reason for hiding this comment

yokuze Apr 28, 2022

Choose a reason for hiding this comment

yokuze Apr 28, 2022

Choose a reason for hiding this comment

yokuze Apr 28, 2022

Choose a reason for hiding this comment

PrestonN commented May 1, 2022

clwy-cn commented Apr 22, 2020 •

edited

Loading

syeopite commented Sep 13, 2021 •

edited

Loading