Add removeTechControlsListener call to add method #2511
Conversation
|
This fixes the immediate issue. Looking into whether we can simplify when |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 97f9ddbbfe978d47c6a109b05fbf0de82d369fe1 (Please note that this is a fully automated comment.) |
1 similar comment
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 97f9ddbbfe978d47c6a109b05fbf0de82d369fe1 (Please note that this is a fully automated comment.) |
|
Apparently re-running savage tests in travis doesn't work. @pam retry |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: c97a5259b9fadf0231ee3f0fd8b3d89c64ce0464 (Please note that this is a fully automated comment.) |
|
Failed with |
gkatsev
force-pushed
the
tech-listeners
branch
from
c97a525
to
b80e2b8
Compare
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: b80e2b8 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: b80e2b8 (Please note that this is a fully automated comment.) |
| @@ -1939,7 +1942,6 @@ class Player extends Component { | |||
| if (this.usingNativeControls_ !== bool) { | |||
There was a problem hiding this comment.
Personally, I would invert this conditional and return early to get rid of a level of nesting in this function. Not worth blocking this PR, though.
There was a problem hiding this comment.
Yeah, and you could do the same with the bool !== undefined above this.
|
Do we have sufficient test coverage for this already? |
|
I'll take a look at what tests we have for this. |
|
I added a test. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: c930090 (Please note that this is a fully automated comment.) |
| player.controls(false); | ||
|
|
||
| player.controls(true); | ||
| equal(listeners, 1, 'addTechControlsListeners should have gotten called once') |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 8276fd9 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 8276fd9 (Please note that this is a fully automated comment.) |
|
Assuming we've tested this on the platforms this was reported on, lgtm. |
|
I've tested it locally on ipads, android native browser, chrome, firefox, safari |
Fixes #2504.