Refactor TextTrackSettings (Phase 2) #3660

misteroneill · 2016-09-30T14:04:21Z

We can review and merge this into the already-approved branch from #3570, then open a new PR for the combination of the two.

Specific Changes proposed

This refactors the TextTrackSettings HTML blob into createEl calls. Also, we extend the createEl function to take a conventional content argument at the end, allowing nested calls to createEl to build up a DOM structure.

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

This plugin makes browserify output numbers instead of the bundle paths to save some bytes in the output.

…d time range The control text element was being counted as the first buffered region but it didn't have the correct styling to be shown. Instead, we keep the buffered region elements in a separate element rather than relying on el.children to be correct.

https://greenkeeper.io/

…#3595) https://greenkeeper.io/

…ideojs#3629) babel-preset-es2015-loose is deprecated as it's now available as an option to the es2015 preset.

* Disable HLS hack on Firefox for Android * Fix canPlayType patching tests * Add test to ensure that canPlayType is not patched in Firefox for Android * Fix assertion message in Firefox for Android test

Some of the code in the html5 tech is a bit redundant and can be written another way to reduce the file size. Also, we made sure that all functions can still be documented properly.

iOS still doesn't have native fullscreen API access. The video element uses the old webkit fullscreen events `webkitbeginfullscreen` and `webkitendfullscreen`. This makes it so both of those trigger `fullscreenchange` on the player always as opposed to only when `requestFullscreen` was called on the player.

* removing duplicate constructor docs fixing doc block (should not have ended in ?) * adding back event doc comments

brandonocasey · 2016-09-30T14:39:16Z

src/js/tracks/text-track-settings.js

+    id: 'captions-foreground-color-%s',
+    label: 'Color',
+    options: [
+      ['#FFF', 'White'],


We should define the colors at the top once as a const, since we are using them in two places

brandonocasey · 2016-09-30T14:42:22Z

src/js/tracks/text-track-settings.js

+    id: 'captions-window-opacity-%s',
+    label: 'Transparency',
+    options: [
+      ['0', 'Transparent'],


maybe define this as a const as well since it is used in two places

brandonocasey · 2016-09-30T14:44:50Z

src/js/tracks/text-track-settings.js

  fontPercent: {
    selector: '.vjs-font-percent > select',
+    id: 'captions-font-size-%s',
+    label: 'Font Size',
+    options: [


why not use an object for options in all of these?

Easier to loop through? plus it matches other usecases above

Also, ordering could be/is important.

brandonocasey · 2016-09-30T14:45:14Z

src/js/tracks/text-track-settings.js

+        'select',
+        {id},
+        undefined,
+        config.options.map(o => {


if options becomes an object this will have to change

brandonocasey · 2016-09-30T14:47:25Z

src/js/tracks/text-track-settings.js

+      {className: 'vjs-tracksettings-colors'},
+      undefined,
+      [
+        createEl(


maybe create all of these elements outside of the main createEl call so that this code is more understandable (as they will all have to have names, and you can add comments)

Yeah, this one could be simpler.

brandonocasey · 2016-09-30T14:47:57Z

src/js/tracks/text-track-settings.js

+      {className: 'vjs-tracksettings-font'},
+      undefined,
+      [
+        createEl(


same comment with this one as the createElColors_ although it isn't nearly as bad.

brandonocasey · 2016-09-30T14:48:39Z

src/js/tracks/text-track-settings.js

+      'div',
+      {className: 'vjs-tracksettings'},
+      undefined,
+      [this.createElColors_(), this.createElFont_(), this.createElControls_()]);


should this ); go on the next line?

I feel like the function argument format we've decided on was:

foo( 'a', 'b', 'c');

brandonocasey · 2016-09-30T14:50:55Z

src/js/tracks/text-track-settings.js

+      undefined,
+      [this.createElColors_(), this.createElFont_(), this.createElControls_()]);
+
+    const heading = createEl(


might just be opinion here but I would like it if these blocks looked something like this:

const heading = createEl('div', { ... }, { ... });

I agree, personally, but this is another case of trying to be consistent is some way w/ function arguments.

brandonocasey · 2016-09-30T14:53:00Z

src/js/tracks/text-track-settings.js

+   * @method setDefaults
+   */
+  setDefaults() {
+    Obj.each(selectConfigs, config => {


I think we typically wrap this in another set of parens in video.js. Obj.each((selectConfigs, config) =>

We're iterating over selectConfigs, though. If anything, it'd be Obj.each(selectConfigs, (config) =>.

But it's apparent that this could be confusing, I'll add parens around config. 👍

brandonocasey · 2016-09-30T14:54:14Z

src/js/tracks/text-track-settings.js

+   */
+  createElControls_() {
+    return createEl(
+      'div',


same comment with this one as the createElColors_ although it isn't nearly as bad.

gkatsev

Assuming the DOM output is the same, don't have any additional comments beyond what @brandonocasey already mentioned.

gkatsev · 2016-09-30T14:57:00Z

src/js/tracks/text-track-settings.js

  fontPercent: {
    selector: '.vjs-font-percent > select',
+    id: 'captions-font-size-%s',
+    label: 'Font Size',
+    options: [


Easier to loop through? plus it matches other usecases above

gkatsev · 2016-09-30T14:57:32Z

src/js/tracks/text-track-settings.js

@@ -108,128 +200,6 @@ function setSelectedOption(el, value, parser) {
  }
 }

-function captionOptionsMenuTemplate(uniqueId, dialogLabelId, dialogDescriptionId) {
-  const template = `


gkatsev · 2016-09-30T16:14:59Z

src/js/utils/dom.js

 * @return {Element}
 * @function createEl
 */
-export function createEl(tagName = 'div', properties = {}, attributes = {}) {
+export function createEl(tagName = 'div', properties = {}, attributes = {}, content) {


This change would make it a minor, which is fine.

But it does seem like a nice thing to have.

)

misteroneill · 2016-10-03T14:37:27Z

To be clear about the timeline/plan here, once this is approved, I'll close an re-open a new PR with both this one and #3570 together.

brandonocasey

LGTM

brandonocasey · 2016-10-03T17:47:22Z

src/js/tracks/text-track-settings.js

-      ['#FF0', 'Yellow'],
-      ['#F0F', 'Magenta'],
-      ['#0FF', 'Cyan']
+      COLOR_BLACK,


gkatsev · 2016-10-03T17:54:07Z

@OwenEdwards would you be able to take a look?

gkatsev · 2016-10-05T21:48:59Z

src/js/tracks/text-track-settings.js

@@ -301,7 +301,7 @@ class TextTrackSettings extends Component {

    return createEl('fieldset', {
      className: 'vjs-fg-color vjs-tracksetting'
-    }, undefined, [legend, select, opacity]);
+    }, undefined, [legend].concat(select, opacity));


why is the other way broken?

Because the select is an array.

Oh, I see now.

…eojs#3669) This updates our changelog automation to use conventinoal changelog. Ultimately, it will make it easier to merge PRs because it won't require the use of the contrib tool. It uses https://github.com/videojs/conventional-changelog-videojs/blob/master/convention.md

Also, replaced usage of assert.ok with assert.strictEqual where possible.

brandonocasey and others added 9 commits September 29, 2016 11:58

chore(deps): add the bundle-collapser browserify plugin

816291e

This plugin makes browserify output numbers instead of the bundle paths to save some bytes in the output.

chore(package): update grunt-shell to version 2.0.0 (videojs#3642)

2032b17

https://greenkeeper.io/

chore(package): update grunt-contrib-cssmin to version 1.0.2 (videojs…

54e3db5

…#3595) https://greenkeeper.io/

chore(package): remove es2015-loose since it's an option for es2015 (v…

c545acd

…ideojs#3629) babel-preset-es2015-loose is deprecated as it's now available as an option to the es2015 preset.

fix: disable HLS hack on Firefox for Android (videojs#3586)

dd2aff0

* Disable HLS hack on Firefox for Android * Fix canPlayType patching tests * Add test to ensure that canPlayType is not patched in Firefox for Android * Fix assertion message in Firefox for Android test

chore: refactor redundant code in html5 tech (videojs#3593)

6878c21

Some of the code in the html5 tech is a bit redundant and can be written another way to reduce the file size. Also, we made sure that all functions can still be documented properly.

chore: refactor redundant or verbose code in player.js (videojs#3597)

ae3e277

* removing duplicate constructor docs fixing doc block (should not have ended in ?) * adding back event doc comments

brandonocasey requested changes Sep 30, 2016

View reviewed changes

gkatsev reviewed Sep 30, 2016

View reviewed changes

OwenEdwards and others added 2 commits September 30, 2016 15:18

test(a11y): add basic accessibility testing using grunt-accessibility

7d85f27

fix(html5): disable manual timeupdate events on html5 tech (videojs#3656

920c54a

)

brandonocasey approved these changes Oct 3, 2016

View reviewed changes

src/js/tracks/text-track-settings.js

['#FF0', 'Yellow'],

['#F0F', 'Magenta'],

['#0FF', 'Cyan']

COLOR_BLACK,

Copy link

Contributor

brandonocasey Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

gkatsev reviewed Oct 5, 2016

View reviewed changes

gkatsev approved these changes Oct 6, 2016

View reviewed changes

gkatsev and others added 12 commits October 6, 2016 14:09

v5.12.3

d6e49a4

DRYer code while keeping tests passing

59d1281

Create obj util and implement it

67a6868

Add prefix to window-color

bb8650f

Bring back old helper functions

8ee39f7

Docblock tweaks

171c446

Fix condition

9240463

Replace massive HTML blob with DOM methods

3124ddd

Add tests for DOM util changes

3192597

Also, replaced usage of assert.ok with assert.strictEqual where possible.

Constant-ize repeated options, localize text content

1912829

Attempt at more readable code

125e959

misteroneill added 2 commits October 11, 2016 15:51

Clarify arguments

7ec6fce

Fix tests

981b64d

misteroneill force-pushed the refactor-tts-2 branch from b70653f to 981b64d Compare October 11, 2016 19:52

misteroneill closed this Oct 11, 2016

misteroneill deleted the refactor-tts-2 branch October 11, 2016 19:54

misteroneill mentioned this pull request Oct 11, 2016

Refactor TextTrackSettings: Complete #3679

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor TextTrackSettings (Phase 2) #3660

Refactor TextTrackSettings (Phase 2) #3660

misteroneill commented Sep 30, 2016

brandonocasey Sep 30, 2016

brandonocasey Sep 30, 2016

brandonocasey Sep 30, 2016

gkatsev Sep 30, 2016

misteroneill Sep 30, 2016 •

edited

Loading

brandonocasey Sep 30, 2016

brandonocasey Sep 30, 2016

misteroneill Sep 30, 2016

brandonocasey Sep 30, 2016

brandonocasey Sep 30, 2016

misteroneill Sep 30, 2016

brandonocasey Sep 30, 2016 •

edited

Loading

misteroneill Sep 30, 2016

brandonocasey Sep 30, 2016

misteroneill Sep 30, 2016

misteroneill Sep 30, 2016

brandonocasey Sep 30, 2016

gkatsev left a comment

gkatsev Sep 30, 2016

gkatsev Sep 30, 2016

gkatsev Sep 30, 2016

gkatsev Sep 30, 2016

misteroneill commented Oct 3, 2016

brandonocasey left a comment

brandonocasey Oct 3, 2016

gkatsev commented Oct 3, 2016

gkatsev Oct 5, 2016

misteroneill Oct 6, 2016

gkatsev Oct 6, 2016

Refactor TextTrackSettings (Phase 2) #3660

Refactor TextTrackSettings (Phase 2) #3660

Conversation

misteroneill commented Sep 30, 2016

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill commented Oct 3, 2016

brandonocasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill Sep 30, 2016 •

edited

Loading

brandonocasey Sep 30, 2016 •

edited

Loading