From 724c214eb0fa4e11f4af52cc519ef073cd60ffdd Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Wed, 24 Aug 2016 14:17:03 -0400 Subject: [PATCH 01/12] DRYer code while keeping tests passing --- src/js/tracks/text-track-settings.js | 265 +++++++++---------- test/unit/tracks/text-track-settings.test.js | 106 +++++--- 2 files changed, 193 insertions(+), 178 deletions(-) diff --git a/src/js/tracks/text-track-settings.js b/src/js/tracks/text-track-settings.js index 4e8e21ab87..f6768baee5 100644 --- a/src/js/tracks/text-track-settings.js +++ b/src/js/tracks/text-track-settings.js @@ -1,12 +1,61 @@ /** * @file text-track-settings.js */ +import window from 'global/window'; import Component from '../component'; -import * as Events from '../utils/events.js'; import * as Fn from '../utils/fn.js'; import log from '../utils/log.js'; -import safeParseTuple from 'safe-json-parse/tuple'; -import window from 'global/window'; + +const LOCAL_STORAGE_KEY = 'vjs-text-track-settings'; + +// Configuration for the various element. +const selectConfigs = { + backgroundColor: { + selector: '.vjs-bg-color > select' + }, + backgroundOpacity: { + selector: '.vjs-bg-opacity > select' + }, + color: { + selector: '.vjs-fg-color > select' + }, + edgeStyle: { + selector: '.vjs-edge-style > select' + }, + fontFamily: { + selector: '.vjs-font-family > select' + }, + fontPercent: { + selector: '.vjs-font-percent > select', + default: 2, + parser: (v) => v === '1.00' ? null : Number(v) + }, + textOpacity: { + selector: '.vjs-text-opacity > select' + }, + windowColor: { + selector: '.window-color > select' + }, + windowOpacity: { + selector: '.vjs-window-opacity > select' + } +}; + +const iterateSelectConfigs = (fn) => { + Object.keys(selectConfigs).forEach(key => { + fn(selectConfigs[key], key); + }); +}; function captionOptionsMenuTemplate(uniqueId, dialogLabelId, dialogDescriptionId) { const template = ` @@ -130,37 +179,6 @@ function captionOptionsMenuTemplate(uniqueId, dialogLabelId, dialogDescriptionId return template; } -function getSelectedOptionValue(target) { - let selectedOption; - - // not all browsers support selectedOptions, so, fallback to options - if (target.selectedOptions) { - selectedOption = target.selectedOptions[0]; - } else if (target.options) { - selectedOption = target.options[target.options.selectedIndex]; - } - - return selectedOption.value; -} - -function setSelectedOption(target, value) { - if (!value) { - return; - } - - let i; - - for (i = 0; i < target.options.length; i++) { - const option = target.options[i]; - - if (option.value === value) { - break; - } - } - - target.selectedIndex = i; -} - /** * Manipulate settings of texttracks * @@ -175,38 +193,28 @@ class TextTrackSettings extends Component { super(player, options); this.hide(); + this.updateDisplay = Fn.bind(this, this.updateDisplay); + // Grab `persistTextTrackSettings` from the player options if not passed in child options if (options.persistTextTrackSettings === undefined) { this.options_.persistTextTrackSettings = this.options_.playerOptions.persistTextTrackSettings; } - Events.on(this.$('.vjs-done-button'), 'click', Fn.bind(this, function() { + this.on(this.$('.vjs-done-button'), 'click', () => { this.saveSettings(); this.hide(); - })); - - Events.on(this.$('.vjs-default-button'), 'click', Fn.bind(this, function() { - this.$('.vjs-fg-color > select').selectedIndex = 0; - this.$('.vjs-bg-color > select').selectedIndex = 0; - this.$('.window-color > select').selectedIndex = 0; - this.$('.vjs-text-opacity > select').selectedIndex = 0; - this.$('.vjs-bg-opacity > select').selectedIndex = 0; - this.$('.vjs-window-opacity > select').selectedIndex = 0; - this.$('.vjs-edge-style select').selectedIndex = 0; - this.$('.vjs-font-family select').selectedIndex = 0; - this.$('.vjs-font-percent select').selectedIndex = 2; + }); + + this.on(this.$('.vjs-default-button'), 'click', () => { + iterateSelectConfigs(config => { + this.$(config.selector).selectedIndex = config.default || 0; + }); this.updateDisplay(); - })); - - Events.on(this.$('.vjs-fg-color > select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-bg-color > select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.window-color > select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-text-opacity > select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-bg-opacity > select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-window-opacity > select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-font-percent select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-edge-style select'), 'change', Fn.bind(this, this.updateDisplay)); - Events.on(this.$('.vjs-font-family select'), 'change', Fn.bind(this, this.updateDisplay)); + }); + + iterateSelectConfigs(config => { + this.on(this.$(config.selector), 'change', this.updateDisplay); + }); if (this.options_.persistTextTrackSettings) { this.restoreSettings(); @@ -236,83 +244,77 @@ class TextTrackSettings extends Component { } /** - * Get texttrack settings - * Settings are - * .vjs-edge-style - * .vjs-font-family - * .vjs-fg-color - * .vjs-text-opacity - * .vjs-bg-color - * .vjs-bg-opacity - * .window-color - * .vjs-window-opacity + * Parses out option values. + * + * @private + * @param {String} value + * @param {Function} [parser] + * Optional function to adjust the value. + * @return {Mixed} + * Will be `undefined` if no value exists (or if given value is "none"). + * @method parseOptionValue_ + */ + parseOptionValue_(value, parser) { + if (parser) { + value = parser(value); + } + + if (value && value !== 'none') { + return value; + } + } + + /** + * Gets an object of text track settings (or null). * - * @return {Object} + * @return {Object|null} * @method getValues */ getValues() { - const textEdge = getSelectedOptionValue(this.$('.vjs-edge-style select')); - const fontFamily = getSelectedOptionValue(this.$('.vjs-font-family select')); - const fgColor = getSelectedOptionValue(this.$('.vjs-fg-color > select')); - const textOpacity = getSelectedOptionValue(this.$('.vjs-text-opacity > select')); - const bgColor = getSelectedOptionValue(this.$('.vjs-bg-color > select')); - const bgOpacity = getSelectedOptionValue(this.$('.vjs-bg-opacity > select')); - const windowColor = getSelectedOptionValue(this.$('.window-color > select')); - const windowOpacity = getSelectedOptionValue(this.$('.vjs-window-opacity > select')); - const fontPercent = window.parseFloat(getSelectedOptionValue(this.$('.vjs-font-percent > select'))); - - const result = { - fontPercent, - fontFamily, - textOpacity, - windowColor, - windowOpacity, - backgroundOpacity: bgOpacity, - edgeStyle: textEdge, - color: fgColor, - backgroundColor: bgColor - }; - - for (const name in result) { - if (result[name] === '' || result[name] === 'none' || (name === 'fontPercent' && result[name] === 1.00)) { - delete result[name]; + let result = null; + + iterateSelectConfigs((config, key) => { + const el = this.$(config.selector); + let value = el.options[el.options.selectedIndex].value; + + value = this.parseOptionValue_(value, config.parser); + + if (value !== undefined) { + result = result || {}; + result[key] = value; } - } + }); + return result; } /** - * Set texttrack settings - * Settings are - * .vjs-edge-style - * .vjs-font-family - * .vjs-fg-color - * .vjs-text-opacity - * .vjs-bg-color - * .vjs-bg-opacity - * .window-color - * .vjs-window-opacity + * Sets text track settings from an object of values. * - * @param {Object} values Object with texttrack setting values + * @param {Object} values * @method setValues */ setValues(values) { - setSelectedOption(this.$('.vjs-edge-style select'), values.edgeStyle); - setSelectedOption(this.$('.vjs-font-family select'), values.fontFamily); - setSelectedOption(this.$('.vjs-fg-color > select'), values.color); - setSelectedOption(this.$('.vjs-text-opacity > select'), values.textOpacity); - setSelectedOption(this.$('.vjs-bg-color > select'), values.backgroundColor); - setSelectedOption(this.$('.vjs-bg-opacity > select'), values.backgroundOpacity); - setSelectedOption(this.$('.window-color > select'), values.windowColor); - setSelectedOption(this.$('.vjs-window-opacity > select'), values.windowOpacity); - - let fontPercent = values.fontPercent; - - if (fontPercent) { - fontPercent = fontPercent.toFixed(2); - } + iterateSelectConfigs((config, key) => { + const value = values[key]; + + if (!value) { + return; + } - setSelectedOption(this.$('.vjs-font-percent > select'), fontPercent); + const el = this.$(config.selector); + + // Find the option that should be selected by comparing value(s) and + // setting the `selectedIndex` of the -
+
Window element. + * + * @param {Object} config + * @param {Function} [parser] + * Optional function to adjust the value. + * @return {Mixed} + */ +function getSelectedOptionValue(el, parser) { + const value = el.options[el.options.selectedIndex].value; + + return parseOptionValue(value, parser); +} + +/** + * Sets the selected
-
- Background - - - - - - -
-
- Window - - - - - - -
- -
-
- - -
-
- - -
-
- - -
-
-
- - -
- - - `; - - return template; -} - /** * Manipulate settings of text tracks * @@ -242,6 +212,7 @@ class TextTrackSettings extends Component { constructor(player, options) { super(player, options); + this.setDefaults(); this.hide(); this.updateDisplay = Fn.bind(this, this.updateDisplay); @@ -257,9 +228,7 @@ class TextTrackSettings extends Component { }); this.on(this.$('.vjs-default-button'), 'click', () => { - Obj.each(selectConfigs, config => { - this.$(config.selector).selectedIndex = config.default || 0; - }); + this.setDefaults(); this.updateDisplay(); }); @@ -272,6 +241,134 @@ class TextTrackSettings extends Component { } } + /** + * Create a elements to their default values. + * + * @method setDefaults + */ + setDefaults() { + Obj.each(selectConfigs, config => { + const index = config.hasOwnProperty('default') ? config.default : 0; + + this.$(config.selector).selectedIndex = index; + }); + } + + /** + * Restore texttrack settings * * @method restoreSettings */ diff --git a/src/js/utils/dom.js b/src/js/utils/dom.js index acb66680f7..936d48b39b 100644 --- a/src/js/utils/dom.js +++ b/src/js/utils/dom.js @@ -96,10 +96,11 @@ export function getEl(id) { * @param {String} [tagName='div'] Name of tag to be created. * @param {Object} [properties={}] Element properties to be applied. * @param {Object} [attributes={}] Element attributes to be applied. + * @param {String|Element|TextNode|Array|Function} [content] Contents for the element (see: `normalizeContent`) * @return {Element} * @function createEl */ -export function createEl(tagName = 'div', properties = {}, attributes = {}) { +export function createEl(tagName = 'div', properties = {}, attributes = {}, content) { const el = document.createElement(tagName); Object.getOwnPropertyNames(properties).forEach(function(propName) { @@ -113,6 +114,11 @@ export function createEl(tagName = 'div', properties = {}, attributes = {}) { has been deprecated. Use the third argument instead. createEl(type, properties, attributes). Attempting to set ${propName} to ${val}.`); el.setAttribute(propName, val); + + // Handle textContent since it's not supported everywhere and we have a + // method for it. + } else if (propName === 'textContent') { + textContent(el, val); } else { el[propName] = val; } @@ -122,6 +128,10 @@ export function createEl(tagName = 'div', properties = {}, attributes = {}) { el.setAttribute(attrName, attributes[attrName]); }); + if (content) { + appendContent(el, content); + } + return el; } @@ -139,6 +149,7 @@ export function textContent(el, text) { } else { el.textContent = text; } + return el; } /** From 0f663a6039e889f5e361e8078bf431d0309cec63 Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Thu, 25 Aug 2016 10:28:39 -0400 Subject: [PATCH 08/12] Add tests for DOM util changes Also, replaced usage of assert.ok with assert.strictEqual where possible. --- test/unit/utils/dom.test.js | 41 ++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/test/unit/utils/dom.test.js b/test/unit/utils/dom.test.js index c9b0c02d96..2ef97a6902 100644 --- a/test/unit/utils/dom.test.js +++ b/test/unit/utils/dom.test.js @@ -15,8 +15,8 @@ QUnit.test('should return the element with the ID', function(assert) { el1.id = 'test_id1'; el2.id = 'test_id2'; - assert.ok(Dom.getEl('test_id1') === el1, 'found element for ID'); - assert.ok(Dom.getEl('#test_id2') === el2, 'found element for CSS ID'); + assert.strictEqual(Dom.getEl('test_id1'), el1, 'found element for ID'); + assert.strictEqual(Dom.getEl('#test_id2'), el2, 'found element for CSS ID'); }); QUnit.test('should create an element', function(assert) { @@ -27,10 +27,27 @@ QUnit.test('should create an element', function(assert) { 'data-test': 'asdf' }); - assert.ok(div.nodeName === 'DIV'); - assert.ok(span.nodeName === 'SPAN'); - assert.ok(span.getAttribute('data-test') === 'asdf'); - assert.ok(span.innerHTML === 'fdsa'); + assert.strictEqual(div.nodeName, 'DIV'); + assert.strictEqual(span.nodeName, 'SPAN'); + assert.strictEqual(span.getAttribute('data-test'), 'asdf'); + assert.strictEqual(span.innerHTML, 'fdsa'); +}); + +QUnit.test('should create an element, supporting textContent', function(assert) { + const span = Dom.createEl('span', {textContent: 'howdy'}); + + if (span.textContent) { + assert.strictEqual(span.textContent, 'howdy', 'works in browsers that support textContent'); + } else { + assert.strictEqual(span.innerText, 'howdy', 'works in browsers that DO NOT support textContent'); + } +}); + +QUnit.test('should create an element with content', function(assert) { + const span = Dom.createEl('span'); + const div = Dom.createEl('div', undefined, undefined, span); + + assert.strictEqual(div.firstChild, span); }); QUnit.test('should insert an element first in another', function(assert) { @@ -39,28 +56,28 @@ QUnit.test('should insert an element first in another', function(assert) { const parent = document.createElement('div'); Dom.insertElFirst(el1, parent); - assert.ok(parent.firstChild === el1, 'inserts first into empty parent'); + assert.strictEqual(parent.firstChild, el1, 'inserts first into empty parent'); Dom.insertElFirst(el2, parent); - assert.ok(parent.firstChild === el2, 'inserts first into parent with child'); + assert.strictEqual(parent.firstChild, el2, 'inserts first into parent with child'); }); QUnit.test('should get and remove data from an element', function(assert) { const el = document.createElement('div'); const data = Dom.getElData(el); - assert.ok(typeof data === 'object', 'data object created'); + assert.strictEqual(typeof data, 'object', 'data object created'); // Add data - const testData = { asdf: 'fdsa' }; + const testData = {asdf: 'fdsa'}; data.test = testData; - assert.ok(Dom.getElData(el).test === testData, 'data added'); + assert.strictEqual(Dom.getElData(el).test, testData, 'data added'); // Remove all data Dom.removeElData(el); - assert.ok(!Dom.hasElData(el), 'cached item emptied'); + assert.notOk(Dom.hasElData(el), 'cached item emptied'); }); QUnit.test('addElClass()', function(assert) { From 3be3bac27c1c2de4b4ba1cb3e4a71294f63db622 Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Mon, 3 Oct 2016 10:55:59 -0400 Subject: [PATCH 09/12] Constant-ize repeated options, localize text content --- src/js/tracks/text-track-settings.js | 66 +++++++++++++++++----------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/src/js/tracks/text-track-settings.js b/src/js/tracks/text-track-settings.js index 2bc57a58a8..8e3b74f4c2 100644 --- a/src/js/tracks/text-track-settings.js +++ b/src/js/tracks/text-track-settings.js @@ -10,6 +10,19 @@ import log from '../utils/log'; const LOCAL_STORAGE_KEY = 'vjs-text-track-settings'; +const COLOR_BLACK = ['#000', 'Black']; +const COLOR_BLUE = ['#00F', 'Blue']; +const COLOR_CYAN = ['#0FF', 'Cyan']; +const COLOR_GREEN = ['#0F0', 'Green']; +const COLOR_MAGENTA = ['#F0F', 'Magenta']; +const COLOR_RED = ['#F00', 'Red']; +const COLOR_WHITE = ['#FFF', 'White']; +const COLOR_YELLOW = ['#FF0', 'Yellow']; + +const OPACITY_OPAQUE = ['1', 'Opaque']; +const OPACITY_SEMI = ['0.5', 'Semi-Transparent']; +const OPACITY_TRANS = ['0', 'Transparent']; + // Configuration for the various