Refactor TextTrackSettings #3570

misteroneill · 2016-08-24T18:25:42Z

Description

The TextTrackSettings component has a lot of limitations: a large, hard-coded HTML string, repetitive code~~, and does not inherit from ModalDialog (which would provide accessibility benefits and more)~~.

This is the first in a series of 2 PRs. This one is focused on reducing repetitive code and making the component DRYer overall. The second PR will be focused on replacing the large HTML string with DOM generation methods. ~~And the third PR will be focused on inheriting from ModalDialog instead of Component.~~

Update: I think that inheriting from ModalDialog would be ideal, but it would also mean too much backward-incompatibility (and too much workaround code to achieve backward-compatibility). Let's reserve that for 6.0.

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

…ideojs#3562

…loses videojs#3538

closes videojs#3565

gkatsev · 2016-08-24T19:38:13Z

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

    let values;

    try {
-      [err, values] = safeParseTuple(window.localStorage.getItem('vjs-text-track-settings'));


I still think it's nice to use safeParseTuple. I think the reason why this is wrapped in a try/catch is to make sure that the call to localStorage doesn't break.

It seems like overkill to have two try blocks (even if one is hidden in a dependency). This way we only have one.

probably not necessary here but it allows us to more easily differentiate between the two errors.
Also, I wonder if safeJSONparse is used anywhere else.

I don't know that it does allow us to easily differentiate. Either way, you have an Error object that gets logged to the console. I suppose before it was sometimes logged as an error and sometimes as a warning, but... that doesn't seem like a significant benefit.

It's used in the Player. I dunno if it's worth including it at all since it's basically just a small wrapper around try/catch... but that's for another discussion/PR.

Also, the other benefit of safejsonparse is encapsulation. Yeah, not much help here but I think it does help in general. Supposedly, try/catch don't hurt much anymore in the latest and greatest js engines but older engines still have issues with deoping the entire function.

gkatsev · 2016-08-24T19:55:08Z

I guess the other parts of the proposal are still to come?

gkatsev · 2016-08-24T19:58:08Z

Also, maybe make each part a separate PR?

misteroneill · 2016-08-25T00:45:41Z

Yeah, this part is going to be focused on DRYness.

…videojs#3574

brandonocasey · 2016-08-25T18:04:29Z

src/js/utils/obj.js

+ *         A callback function which is called for each key in the object. It
+ *         receives the value and key as arguments.
+ */
+export function each(object, fn) {


how much space would it take to bring in lodash here instead? (could provide some lodash code sharing between projects if it is not a lot more)

I think it'd be a lot more. I'm willing to check to see how much more, but I suspect it's quite a bit (comparatively).

brandonocasey · 2016-08-25T18:55:15Z

need to add a description to the mentioned doc block
need to check how much size lodash will add gzipped

Other than that LGTM

misteroneill · 2016-08-25T19:22:03Z

Doc block is updated.

As for adding lodash, we already have lodash-compat in place. Using its methods - each() and reduce() - adds 2.5kb to the gzipped size. Removing lodash-compat and using lodash 4 instead adds 6.5kb. This is likely because lodash has multiple internal dependencies.

brandonocasey · 2016-08-25T19:36:36Z

ok yeah maybe we can revisit it at a later time, when we have a better way to share dependencies between projects

gkatsev · 2016-08-25T19:44:55Z

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

@@ -352,18 +358,18 @@ class TextTrackSettings extends Component {
    const values = this.getValues();

    try {
-      if (Object.getOwnPropertyNames(values).length > 0) {
-        window.localStorage.setItem('vjs-text-track-settings', JSON.stringify(values));
+      if (values) {


why remove the check for the size of values?

The default return value for getValues is an object which is truthy, so, we'll never enter the else

Good catch. That was from some earlier work where getValues() was not always returning an object.

OwenEdwards · 2016-08-31T06:09:22Z

I don't see any changes that impact a11y. LGTM.

(Hoping we get rid of that chunk of HTML for the dialog at some point soon, too ;-) )

misteroneill · 2016-08-31T18:03:30Z

@OwenEdwards Yep! That's the next PR!

…s#3640

…2.6. closes videojs#3634

…ixes videojs#3283. closes videojs#3648

Fixes videojs#3587. Some commented out css was removed as it is no longer necessary and potentially causes issues.

ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE were both moved to a .github/ folder to clean up the root of the project a bit. CONTRIBUTING.md was kept at the top level because it is generic enough and contains useful information.

Updates `object.assign` to the latest, and uses a semver range so that consumers can `npm dedupe`.

misteroneill · 2016-09-30T13:55:36Z

Going to close this and re-open against master when the other branch gets included.

misteroneill and others added 4 commits August 24, 2016 11:00

@misteroneill fixed wrapping native and emulated MediaErrors. closes v…

b56ed61

…ideojs#3562

@snyderizer fixed switching between audio tracks. Fixes videojs#3510. c…

23d1fcd

…loses videojs#3538

@jbarabander added title attribute to audio button. Fixes videojs#3528.

46d7135

closes videojs#3565

@misteroneill fixed IE8 media error test failure. closes videojs#3568

1d2d085

gkatsev reviewed Aug 24, 2016
View reviewed changes

misteroneill force-pushed the refactor-tts branch from c8c7bda to 5c7fa90 Compare August 24, 2016 23:18

jbarabander added 2 commits August 25, 2016 11:01

@jbarabander updated Audio to Audio Track and added to en.json. closes …

a2167a6

…videojs#3574

v5.11.5

eff1cf3

misteroneill changed the base branch from master to refactor-tts-complete August 25, 2016 15:55

misteroneill force-pushed the refactor-tts branch from ce4ccba to 38c83c0 Compare August 25, 2016 15:58

Merge branch 'stable'

f373837

brandonocasey reviewed Aug 25, 2016
View reviewed changes

misteroneill added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Aug 25, 2016

@gkatsev added es2015 babel preset as a dependency. closes videojs#3577

0b10b69

gkatsev reviewed Aug 25, 2016
View reviewed changes

@mister-ben added task to maintain translations. closes videojs#3572

94f22bb

gkatsev approved these changes Sep 20, 2016

View reviewed changes

gkatsev added this to the 5.12 milestone Sep 27, 2016

gkatsev and others added 19 commits September 28, 2016 13:18

@gkatsev checked throwIfWhitespace first in hasElClass. closes videoj…

282bda7

…s#3640

@misteroneill pinned grunt-contrib-uglify to ~0.11 to pin uglify to ~…

ecc9533

…2.6. closes videojs#3634

@gkatsev set playerId on new el created for movingMediaElementInDOM. F…

3a92752

…ixes videojs#3283. closes videojs#3648

v5.11.7

d520d29

Merge branch 'stable'

7cf2de3

Update CHANGELOG.md

057588f

v5.12.2

5f95e41

fix(css): remove commented out css

5fdcd46

Fixes videojs#3587. Some commented out css was removed as it is no longer necessary and potentially causes issues.

chore: move metadata to hidden folder and update references

86f0830

ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE were both moved to a .github/ folder to clean up the root of the project a bit. CONTRIBUTING.md was kept at the top level because it is generic enough and contains useful information.

chore: update object.assign to ^4.0.4

08c7f4e

Updates `object.assign` to the latest, and uses a semver range so that consumers can `npm dedupe`.

feat(lang): add missing translations to el.json

eb0efd4

feat(lang): add missing translations in fr.json

280ecd4

docs: fix broken links in docs index.md

4063f96

DRYer code while keeping tests passing

a2c57b7

Create obj util and implement it

277ff01

Add prefix to window-color

0cd9e8e

Bring back old helper functions

3454083

Docblock tweaks

ef19623

Fix condition

f693dd3

misteroneill force-pushed the refactor-tts branch from 58c241f to f693dd3 Compare September 29, 2016 01:32

misteroneill closed this Sep 30, 2016

misteroneill mentioned this pull request Sep 30, 2016

Refactor TextTrackSettings (Phase 2) #3660

Closed

2 tasks

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

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 #3570

Refactor TextTrackSettings #3570

misteroneill commented Aug 24, 2016 •

edited

Loading

gkatsev Aug 24, 2016

misteroneill Aug 24, 2016

gkatsev Aug 25, 2016

misteroneill Aug 25, 2016

gkatsev Aug 25, 2016

gkatsev commented Aug 24, 2016

gkatsev commented Aug 24, 2016

misteroneill commented Aug 25, 2016

brandonocasey Aug 25, 2016

misteroneill Aug 25, 2016

brandonocasey commented Aug 25, 2016 •

edited

Loading

misteroneill commented Aug 25, 2016

brandonocasey commented Aug 25, 2016

gkatsev Aug 25, 2016

gkatsev Aug 25, 2016

misteroneill Aug 25, 2016

OwenEdwards commented Aug 31, 2016

misteroneill commented Aug 31, 2016

misteroneill commented Sep 30, 2016

Refactor TextTrackSettings #3570

Refactor TextTrackSettings #3570

Conversation

misteroneill commented Aug 24, 2016 • edited Loading

Description

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

Choose a reason for hiding this comment

gkatsev commented Aug 24, 2016

gkatsev commented Aug 24, 2016

misteroneill commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Aug 25, 2016 • edited Loading

misteroneill commented Aug 25, 2016

brandonocasey commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenEdwards commented Aug 31, 2016

misteroneill commented Aug 31, 2016

misteroneill commented Sep 30, 2016

misteroneill commented Aug 24, 2016 •

edited

Loading

brandonocasey commented Aug 25, 2016 •

edited

Loading