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.

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

GPII-3285: Support for dynamically injected videos #13

Merged
merged 10 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@jobara
Copy link
Collaborator

jobara commented Aug 17, 2018

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Aug 17, 2018

@amb26 would you be able to review this PR?

// add dynamically added videos
var observer = new MutationObserver(function (mutationRecords) {
mutationRecords.forEach(function (mutationRecord) {
mutationRecord.addedNodes.forEach(function (node) {

This comment has been minimized.

Copy link
@amb26

amb26 Sep 16, 2018

Member

What if videos have been destroyed?

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Sep 17, 2018

@amb26 now handling videos that are removed. I've also updated the jsfiddle to test with. This is ready for more review.

container.append(video);
});

jqUnit.asyncTest("Test gpii.uioPlus.captions.createPlayers - removal", function () {

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

I get a test failure on this one -


Test gpii.uioPlus.captions.createPlayers - removal: Players for each video should have been created

Expected: | [   {     "videoElm": {}   },   {     "videoElm": {}   },   {     "videoElm": {}   },   {     "videoElm": {       "allowTransparency": "true"     }   } ]
-- | --
[   {     "videoElm": {}   },   {     "videoElm": {}   },   {     "videoElm": {}   } ]
[    {      "videoElm":  {}    },    {      "videoElm":  {}    },    {      "videoElm":  {}   },   {     "videoElm": {       "allowTransparency": "true"     }    }  ]
at Object.assertDeepEq (http://localhost:7357/node_modules/infusion/tests/test-core/jqUnit/js/jqUnit.js:180:23)     at Object.<anonymous> (http://localhost:7357/tests/browser/js/captionsEnactorTests.js:273:20)     at Test.run (http://localhost:7357/node_modules/infusion/tests/lib/qunit/js/qunit.js:207:18)     at http://localhost:7357/node_modules/infusion/tests/lib/qunit/js/qunit.js:365:10     at process (http://localhost:7357/node_modules/infusion/tests/lib/qunit/js/qunit.js:1457:24)     at http://localhost:7357/node_modules/infusion/tests/lib/qunit/js/qunit.js:483:5

This comment has been minimized.

Copy link
@amb26

amb26 Sep 26, 2018

Member

Can reproduce in the standard Windows 10 vagrant box

This comment has been minimized.

Copy link
@jobara

jobara Oct 3, 2018

Author Collaborator

Seems like my search for video elements using just $("iframe") was too general. I think the npm test run must add an iframe to the file for reporting or something. I've added a class on those elements that i'm looking for to scope things better.

@amb26

This comment has been minimized.

Copy link
Member

amb26 commented Sep 26, 2018

As well as test failure, could we have a minor dependency update, particularly to the latest gpii-grunt-lint-all which runs much faster, thanks

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Oct 3, 2018

@amb26 the tests should be passing now. I've also updated to the latest gpii-grunt-lint-all. Ready for more review.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

the-t-in-rtf commented Oct 4, 2018

Glad you were able to figure out the testem problems, @jobara.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Oct 4, 2018

@the-t-in-rtf i hadn't actually got the update for testem to work till just now. I took your advice and switched from using "skip" to "launch" and things seem to be working.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

jobara commented Oct 11, 2018

@amb26 I think you merged this, but it still appears to be open.

@amb26

This comment has been minimized.

Copy link
Member

amb26 commented Oct 11, 2018

It looks like I merged a version that omitted your final commit eccef4b, I'll merge again

@amb26 amb26 merged commit eccef4b into GPII:master Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.