refactor redundant code in html5 tech #3593
Conversation
|
Personally, I think this is better and the byte savings are a nice added bonus. 👍 Will review in more detail later. |
|
We probably should add these functions to the prototype programmatically, not in the constructor. |
|
Yeah, that sounds like a good idea. |
|
Will we lose documentation for these functions? |
|
we will lose documentation for these methods |
|
maybe we can keep the comments without keeping the code? |
|
There may be a way of documenting them that I'm not aware of (I've tried a few things in the past), but one thing you can definitely do is something like this: /**
* Begins playback.
*
* @method play
* @memberof Player
*/
Player.prototype.play;Not sure that's exactly it, but it's close. I'm not sure if that'd get dropped by uglify or not. |
|
Another alternative would be to do keep the methods as noops by default: /**
* Begins playback.
*/
play() {}Of course, that probably negates the byte savings. |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: e00add00aa8192c7e2e517f832d5cba9a9820e59 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: b38b45e (Please note that this is a fully automated comment.) |
slight documentation updates to follow jsdoc conventions
|
@dmlap @misteroneill added the doc-comments back in for these methods without extra code |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 3ec4ccb (Please note that this is a fully automated comment.) |
| * | ||
| * @method Html5.prototype.paused | ||
| * @return {Boolean} | ||
| */ |
There was a problem hiding this comment.
Nice. Glad to see this works! 👍
|
LGTM |
brandonocasey
added
needs: LGTM
|
LGTM |
misteroneill
added
confirmed
and removed
needs: LGTM
fix removeTrack typo
| /** | ||
| * Play for html5 tech | ||
| * | ||
| * @method play |
There was a problem hiding this comment.
does jsdoc figure out this @method itself?
There was a problem hiding this comment.
I believe that is the case.
| */ | ||
| 'playbackRate' | ||
| ].forEach(function(prop) { | ||
| Html5.prototype[`set${toTitleCase(prop)}`] = function(v) { |
There was a problem hiding this comment.
might be more readable to just do 'set' + toTitleCase(prop).
Description
Specific Changes proposed
Requirements Checklist