Added the possibility to specify the id of the document element that the subtitles are appended to. #383

Open
wants to merge 4 commits into
from

Projects

None yet

2 participants

@aldatsa

I think that this fixes the issue #382

@ScottDowne ScottDowne commented on an outdated diff Feb 20, 2014
modules/parser/popcorn.parser.js
@@ -39,12 +39,18 @@
parseFn,
parser = {};
- parseFn = function( filename, callback ) {
+ parseFn = function( filename, callback, target ) {
@ScottDowne
ScottDowne Feb 20, 2014

I like the idea, and I appreciate the patch :)

Only comment is what if we made target an options object, with keys and values.

Example:

pop.parseSRT( "my-video.srt", {
target: "my-div"
});

It works well for optional parameters because order doesn't matter. It also means in the future it is going to be easy to add other properties.

Also means custom parsers and plugins could use it for properties other than just target.

Thoughts?

@aldatsa

Great idea! That way is much better 👍
I made a new commit. I substituted target with an options object, with keys and values, as you said @ScottDowne.

@ScottDowne

Just a bit of indenting fixups needed here.

@ScottDowne

To be like the rest of the file, do you mind adding a bit of whitespace before options and around "target" and after [ "target" ]

So like this:

if ( options && options[ "target" ] ) {

For better or worse, that's how we currently do it.

@aldatsa

No problem. Consistency is a good thing.

@ScottDowne ScottDowne commented on an outdated diff Feb 20, 2014
parsers/parserSRT/popcorn.parserSRT.js
@@ -84,6 +84,11 @@
// Later modified by kev: http://kevin.deldycke.com/2007/03/ultimate-regular-expression-for-html-tag-parsing-with-php/
sub.text = sub.text.replace( /&lt;(\/?(font|b|u|i|s))((\s+(\w|\w[\w\-]*\w)(\s*=\s*(?:\".*?\"|'.*?'|[^'\">\s]+))?)+\s*|\s*)(\/?)&gt;/gi, "<$1$3$7>" );
sub.text = sub.text.replace( /\\N/gi, "<br />" );
+
+ if ( options && options[ "target" ] ) {
+ sub.target = options[ "target" ];
+ }
@ScottDowne
ScottDowne Feb 20, 2014

And do you mind moving these tabs to two spaces?

@ScottDowne
ScottDowne Feb 20, 2014

If you don't mind I can do it when I merge this in.

@ScottDowne

I landed this just now. Doesn't seem to be closing the pull request because I did a rebase with current master.

You can see the commit on master: 5835bbf

I appreciate the contribution :)

The next step is to add a test and write some docs :) I'm willing to do all that if you don't mind, but you're welcome to give it go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment