Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use minifiable player methods in fullscreenToggle #851

Closed
wants to merge 6 commits into from
Closed

Don't use minifiable player methods in fullscreenToggle #851

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f9481a
Move player id generation
dmlap Nov 25, 2013
df0eb32
Use qunitjs through npm
dmlap Nov 26, 2013
6823767
Fix typo in test description
dmlap Nov 26, 2013
9aeeb49
Don't use minifiable player methods in fullscreenToggle
dmlap Nov 26, 2013
41949bd
Merge branch 'hotfix/missing-ids' into hotfix/internal-fullscreen-apis
dmlap Nov 26, 2013
ce187b6
Use externs to prevent obfuscation
dmlap Nov 26, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ module.exports = function(grunt) {
},
tests: {
src: ['build/files/combined.video.js', 'build/compiler/goog.base.js', 'src/js/exports.js', 'test/unit/*.js', '!test/unit/api.js'],
externs: ['src/js/media/flash.externs.js', 'test/qunit/qunit-externs.js'],
externs: ['src/js/media/flash.externs.js', 'test/qunit-externs.js'],
dest: 'build/files/test.minified.video.js'
}
},
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"grunt-contrib-less": "~0.6.4",
"grunt-karma": "~0.4.4",
"karma-qunit": "~0.0.2",
"videojs-doc-generator": "0.0.1"
"videojs-doc-generator": "0.0.1",
"qunitjs": "~1.12.0"
},
"testling": {
"browsers": [
Expand Down
6 changes: 3 additions & 3 deletions src/js/control-bar/fullscreen-toggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ vjs.FullscreenToggle.prototype.buildCSSClass = function(){
};

vjs.FullscreenToggle.prototype.onClick = function(){
if (!this.player_.isFullScreen) {
this.player_.requestFullScreen();
if (!this.player_['isFullScreen']) {
Copy link
Member

Choose a reason for hiding this comment

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

meh. At least it'll work now.

Copy link
Member Author

Choose a reason for hiding this comment

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

This may fix that iOS fullscreen control issue (I forget the issue number offhand) but my actual use-case is for custom controls during ad playback.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, just don't like that we had to do this whole string thing.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's ugly. I actually think we could set up an externs file for the player like we do with Flash.
https://github.com/videojs/video.js/blob/master/src/js/media/flash.externs.js

Externs are meant for external projects, but we're running into inter-component issues here, and I think it would avoid the duplicate methods that exporting creates. Want to try that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened an externs-based PR: #853

this.player_['requestFullScreen']();
this.el_.children[0].children[0].innerHTML = 'Non-Fullscreen'; // change the button text to "Non-Fullscreen"
} else {
this.player_.cancelFullScreen();
this.player_['cancelFullScreen']();
this.el_.children[0].children[0].innerHTML = 'Fullscreen'; // change the button to "Fullscreen"
}
};
5 changes: 3 additions & 2 deletions test/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
<!--<![endif]-->

<!-- QUnit -->
<link rel="stylesheet" href="../test/qunit/qunit/qunit.css" />
<script src="../test/qunit/qunit/qunit.js"></script>
<link rel="stylesheet" href="../node_modules/qunitjs/qunit/qunit.css" />
<script src="../node_modules/qunitjs/qunit/qunit.js"></script>

<!-- Video.js CSS -->
<link rel="stylesheet" href="../build/files/video-js.css" type="text/css">
Expand Down Expand Up @@ -66,6 +66,7 @@
<div>
<h1 id="qunit-header">Video.js Test Suite</h1>
<h2 id="qunit-banner"></h2>
<div id="qunit-testrunner-toolbar"></div>
<h2 id="qunit-userAgent"></h2>
<ol id="qunit-tests"></ol>
<div id="qunit-fixture"></div>
Expand Down
5 changes: 3 additions & 2 deletions test/minified-api.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
<title>Video.js Test Suite</title>

<!-- QUnit -->
<link rel="stylesheet" href="../test/qunit/qunit/qunit.css" />
<script src="../test/qunit/qunit/qunit.js"></script>
<link rel="stylesheet" href="../node_modules/qunitjs/qunit/qunit.css" />
<script src="../node_modules/qunitjs/qunit/qunit.js"></script>

<!-- Video.js CSS -->
<link rel="stylesheet" href="../build/files/video-js.css" type="text/css">
Expand Down Expand Up @@ -38,6 +38,7 @@
<div>
<h1 id="qunit-header">Video.js Test Suite</h1>
<h2 id="qunit-banner"></h2>
<div id="qunit-testrunner-toolbar"></div>
<h2 id="qunit-userAgent"></h2>
<ol id="qunit-tests"></ol>
<div id="qunit-fixture"></div>
Expand Down
5 changes: 3 additions & 2 deletions test/minified.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
<title>Video.js Test Suite</title>

<!-- QUnit -->
<link rel="stylesheet" href="../test/qunit/qunit/qunit.css" />
<script src="../test/qunit/qunit/qunit.js"></script>
<link rel="stylesheet" href="../node_modules/qunitjs/qunit/qunit.css" />
<script src="../node_modules/qunitjs/qunit/qunit.js"></script>

<!-- Video.js CSS -->
<link rel="stylesheet" href="../build/files/video-js.css" type="text/css">
Expand All @@ -22,6 +22,7 @@
<div>
<h1 id="qunit-header">Video.js Test Suite</h1>
<h2 id="qunit-banner"></h2>
<div id="qunit-testrunner-toolbar"></div>
<h2 id="qunit-userAgent"></h2>
<ol id="qunit-tests"></ol>
<div id="qunit-fixture"></div>
Expand Down
0 test/qunit/qunit-externs.js → test/qunit-externs.js
View file
Open in desktop
File renamed without changes.
7 changes: 0 additions & 7 deletions test/qunit/.gitignore
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions test/qunit/README.md
View file
Open in desktop

This file was deleted.

16 changes: 0 additions & 16 deletions test/qunit/addons/canvas/README.md
View file
Open in desktop

This file was deleted.

76 changes: 0 additions & 76 deletions test/qunit/addons/canvas/canvas-test.js
View file
Open in desktop

This file was deleted.

19 changes: 0 additions & 19 deletions test/qunit/addons/canvas/canvas.html
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions test/qunit/addons/canvas/qunit-canvas.js
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions test/qunit/addons/close-enough/README.md
View file
Open in desktop

This file was deleted.

37 changes: 0 additions & 37 deletions test/qunit/addons/close-enough/close-enough-test.js
View file
Open in desktop

This file was deleted.

18 changes: 0 additions & 18 deletions test/qunit/addons/close-enough/close-enough.html
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions test/qunit/addons/close-enough/qunit-close-enough.js
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions test/qunit/addons/composite/README.md
View file
Open in desktop

This file was deleted.

33 changes: 0 additions & 33 deletions test/qunit/addons/composite/composite-demo-test.html
View file
Open in desktop

This file was deleted.

Loading