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

FLUID-6296: YouTube captions preference #914

Merged
merged 16 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@jobara
Copy link
Member

commented Jul 23, 2018

Adding a preference and enactor to turn on/off captions in an embedded YouTube video.

https://issues.fluidproject.org/browse/FLUID-6296

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2018

@the-t-in-rtf would you be able to look at the test reports. I'm noticing that they don't include the tests for the news files (e.g. CaptionsPanel.js and CaptionsEnactor.js).

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

Hi, @jobara. They aren't showing up in the report because you didn't add them to the "all tests" rollup. I added them there to confirm and reran, and sure enough, the results showed up.

For the sake of completeness, you should also add each of the new test fixtures to the preferences rollup. It's not used in the automated tests, but devs in the larger group do use them to troubleshoot problems.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2018

🤦 thanks @the-t-in-rtf

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2018

@amb26 could you please review this PR

// After FLUID-6148 (https://issues.fluidproject.org/browse/FLUID-6148) is complete. It should be possible to
// just source the dynamic components with the videos selector. The framework should at that time be able to
// handle the asynchrony of waiting for YT in the expander that creates the player.
fluid.prefs.enactor.captions.waitForYouTubeAPI = function (that) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 24, 2018

Member

Standard docs would be helpful - say what is returned and what aspects of the component are relied on

}
},
events: {
onVideoElmLocated: null

This comment has been minimized.

Copy link
@amb26

amb26 Jul 24, 2018

Member

I don't think the abbreviation of "Element" to "Elm" hlps readability

promise.resolve();
} else {
// the YouTube iframe api will call onYouTubeIframeAPIReady after the api has loaded
window.onYouTubeIframeAPIReady = promise.resolve;

This comment has been minimized.

Copy link
@amb26

amb26 Jul 24, 2018

Member

Rather than blindly bung this onto a global, please make a singleton component using resolveRootSingle which mediates this interaction

};

/**
* Adds the "enabledjsapi=1" query paramater to the query string at the end of the src attribute.

This comment has been minimized.

Copy link
@amb26

amb26 Jul 24, 2018

Member

Typo parameter


/**
* Adds the "enabledjsapi=1" query paramater to the query string at the end of the src attribute.
* If "enabledjsapi" already exists it will modify its value to 1. This is required for API access

This comment has been minimized.

Copy link
@amb26

amb26 Jul 24, 2018

Member

Typo enabled

*
* @return {Object} - An Object representation of the key/value pairs from the query string.
*/
fluid.prefs.enactor.captions.youTubePlayer.parseQueryString = function (queryStr) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 24, 2018

Member

I believe this function is not required in the modern world - https://developer.mozilla.org/en-US/docs/Web/API/URL

This comment has been minimized.

Copy link
@jobara

jobara Jul 25, 2018

Author Member

It doesn't look like the URL API is supported in IE 11 ( https://caniuse.com/#search=URL ). I'll leave a comment that this is deprecated and should be switched to use the URL API after IE 11 is no longer supported.

This comment has been minimized.

Copy link
@amb26

amb26 Jul 25, 2018

Member

A cleaner route would be to just axe this impl now and configure one of the available API polyfills for use on IE11 - e.g. https://www.npmjs.com/package/url-polyfill

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

@amb26 this PR is ready for more review.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

@amb26 suggested that the singleton should be responsible for creating the promise to determine if the YT API is available. See: https://botbot.me/freenode/fluid-work/2018-07-25/?msg=102446834&page=1

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

@amb26 this PR is ready for another round of review

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

@amb26 I have added the polyfill and switched to using the URL api from the browser to handle the video source url. The PR is ready for more review.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

@jobara, I've just cut a new dev release of gpii-grunt-lint-all that has global excludes by default. I have tested it in my own work on separate projects, but would appreciate it if you could quickly update to 1.0.1-dev.20180731T094032Z.112cde7 and confirm that you're able to remove the excludes you added in the other pull.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

@the-t-in-rtf I tested out your dev release of gpii-grunt-lint-all ( 1.0.1-dev.20180731T094032Z.112cde7 ). I'm still finding that "**/*.js" will go into the node_modules for the lintspaces test, "**/*.md" goes into node_modules for the markdownlint test and "**/*.json" goes into node_modules for the jsonlint task.

I committed the updated config for fluid-publish into a branch.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

Thanks, @jobara. I took a few minutes to figure out what was going on in your branch.

For starters, I refined the gpii-grunt-lint-all plugin to exclude .DS_Store files by default, update to 1.0.1-dev.20180801T111658Z.da9c3ba to pick that up. I then discovered a few things about the pattern matching, which ultimately relies on minimatch.

First, pattern precedence is left to right, and you can reintroduce excluded material further to the right. They seem to think of this as a feature, which you can use to twiddle the order of files, by excluding them out and then bringing them back in in a particular order. To deal with this, I made sure the new ignores were the right-most material in my rollup definitions.

However, it seems that it's also possible to break out of negated patterns. So, here's what you had:

        lintAll: {
            sources: {
                md: [ "**/*.md"],
                js: ["**/*.js"],
                json: ["**/*.json"],
                other: ["./.*", "!./**/.DS_Store"]
            }
        }

The pattern **/*.md seems to bypass the inherited !./node_modules/**/* negation, and bring undesirable content from node_modules into scope. Although we might be able to track this down further and report a bug against minimatch, for now the solution is to recognise the dangers of broad patterns like **/*.js and use more targeted settings like the following, which I tested with your branch:

        lintAll: {
            sources: {
                md: [ "./*.md"],
                js: ["./tests/**/*.js", "./*.js"],
                json: ["./tests/**/*.json", "./*.json"],
                other: ["./.*"]
            }
        }

That configuration should lint everything that needs to be linted, and ignore everything that should be ignored.

While I was trying out your branch, I also noted that the bit of the file that loads the package.json is no longer used, and can be removed.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

@the-t-in-rtf thanks for looking into this. I've filed a new JIRA to updated gpii-grunt-lint-all in fluid-publish. https://issues.fluidproject.org/browse/FLUID-6313.

"{that}.options.testOpts.tracklist.1",
false
],
changeEvent: "{youtTubePlayer}.applier.modelChanged",

This comment has been minimized.

Copy link
@amb26

amb26 Aug 1, 2018

Member

Global typo youtTubePlayer

@jobara

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

@amb26 I fixed the typo. This PR is ready for more review.

@amb26 amb26 merged commit 11f62e5 into fluid-project:master Aug 1, 2018

8 checks passed

buildkite/infusion Build #354 passed (31 minutes, 6 seconds)
Details
buildkite/infusion/browser-tests Passed (8 minutes, 2 seconds)
Details
buildkite/infusion/build Passed (8 minutes, 21 seconds)
Details
buildkite/infusion/cleanup Passed (7 seconds)
Details
buildkite/infusion/code-linter Passed (18 seconds)
Details
buildkite/infusion/node-tests Passed (25 seconds)
Details
buildkite/infusion/pipeline Passed (9 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
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.