#91 ✓ checked-in
Brett Gaylor

URLS for person tags

Reported by Brett Gaylor | June 11th, 2010 @ 04:47 PM | in 0.4

We should have have associated URLS with people that are tagged in the video - can go into DIV 3 for now, or could also try this in DIV 4 right beside the name of the person, ie In this video: Celine (http://www.twitter.com/celinesceline)

Comments and changes to this ticket

  • Daniel H

    Daniel H July 16th, 2010 @ 06:09 PM

    • Milestone cleared.
    • Milestone order changed from “0” to “0”
  • annasob

    annasob November 3rd, 2010 @ 11:29 AM

    • Milestone cleared.

    @@@ @@@ [bulk edit]

  • annasob

    annasob November 3rd, 2010 @ 11:30 AM

    • Milestone set to 0.3 release date
  • annasob

    annasob January 12th, 2011 @ 04:05 PM

    • Tag set to plugin, starter
    • Milestone order changed from “1” to “0”
  • Scott Downe

    Scott Downe January 18th, 2011 @ 10:34 AM

    • Tag cleared.
    • Assigned user cleared.
  • Min

    Min January 20th, 2011 @ 11:26 AM

    • State changed from “new” to “assigned”
    • Assigned user set to “Min”
  • annasob

    annasob February 8th, 2011 @ 05:32 PM

    • Milestone changed from 0.3 release date to 0.4
    • Milestone order changed from “5” to “0”
  • Min

    Min February 24th, 2011 @ 10:24 PM

    • State changed from “assigned” to “peer-review-requested”

    Branch tagthisperson

    Updated the implementation of plugin, the html file of the example and the unit test in plugins/tagthisperson

    Test Summary:
    * lint: no errors * lint-plugins: no errors * test/index.html;
    * In Chrome 9.0: Tests completed in 19936 milliseconds. 197 tests of 215 passed, 18 failed.(Mostly related to xhr). * In Firefox 3.6: Tests completed in 21413 milliseconds. 215 tests of 217 passed, 2 failed.

    I'm not sure if these fails are normal or not, since I got the same result in a clean cloned repository of 0.4.
    I also changed the unit tests based on the unit tests of attribution plugin. Not sure if it was a right thing to do or not.

  • Scott Downe

    Scott Downe February 25th, 2011 @ 10:49 AM

    • State changed from “peer-review-requested” to “under-review”

    if you have failing tests on chrome with just xhr, it is most likely due to file security.

    You can solve it by changing chrome's security, or running the test suite through a webserver.

    lint-plugins: no errors
    you mean no new errors? because I think there are lint-plugin errors on 0.4.

  • Scott Downe

    Scott Downe February 25th, 2011 @ 12:09 PM

    • State changed from “under-review” to “review-needs-work”

    There are no more commas between names, and there seems to be an extra space. I want comma separated names with one space.

    Also, I noticed you removed "tagdiv2". That was there to show the functionality of adding people to multiple tags.

    The links work, the code itself works and as far as I can tell is stylistically sound with the rest of the project.

  • Min

    Min February 25th, 2011 @ 12:25 PM

    You were right about the lint-plugins. I just noticed that the message was JSLint check passed.
    There are 3 Errors. My mistake.

    I added --disable-web-security to Chrome, it fixed everything except one that as you mentioned looks like it's just because of web server (jsonp requests require a webserver with php). So, I'll work on IIS.

    For firefox, I'm getting these 2 errors, that I don't know how to fix:
    1. Popcorn: API (1, 3, 4)
    3.Popcorn API creates only 1 global reference, expected: 121 result: 122, diff: 121 122 32. Popcorn Test Runner End: Last Check (1, 0, 1)
    1. Popcorn API did not leak, expected: 121 result: 122, diff: 121 122

    I'll fix comma separation.
    About <div> elements. We actually discussed it with Anna yesterday, that it seems better to have one parent <div> and then just create extra <div> elements in _setup for each person as they are passed to the plugin. Or unless you want to categorize persons in subtitle and have parent <div>s specific to each category not to the person..., maybe.

  • Scott Downe

    Scott Downe February 25th, 2011 @ 12:45 PM

    I am actually OK with this:

    About <div> elements.
    We actually discussed it with Anna yesterday, that it seems better to have one parent <div> and then just create extra <div> elements in _setup for each person as they are passed to the plugin.
    Or unless you want to categorize persons in subtitle and have parent <div>s specific to each category not to the person..., maybe.
    

    That is how it should be.

    I just think the example file footnote.html to show that there is a different when used in two targets. Just add back <div id="tagdiv2" width="50%" height="50%"> and make the Mike track use that second div as the target, and everything works.

  • Min

    Min February 25th, 2011 @ 12:55 PM

    Ok, I'll work on it.

  • Min

    Min February 25th, 2011 @ 11:41 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Branch tagthisperson

    Fixed the comma separator and added the extra parent div elements to the tagthisperson/tagthisperson.html file

    Test Summary:
    lint: no errors.
    lint-plugins: JSLint check passed for tagthisperson plugin - errors for other plugins.
    test/index.html (Ran on Wamp server - Scott, this web server was really fast and simple to setup and use!)
    * On Chrome 9.0: 212 tests of 222 passed. Some tests related to Popcorn.plugin (data.trackEvents)failed. They also failed on a clean repository. I'm getting these errors while running on the web server not from local. * On Firefox 3.6: 224 tests of 224 passed.

  • Scott Downe

    Scott Downe February 28th, 2011 @ 10:39 AM

    • State changed from “peer-review-requested” to “under-review”
    • Assigned user changed from “Min” to “Scott Downe”
  • Scott Downe

    Scott Downe February 28th, 2011 @ 11:35 AM

    • State changed from “under-review” to “review-needs-work”

    You are showing a comma after a name if it is the only name. Two names work correctly.

    You are also never removing the comma, so if the user seeks back and forth, the comma gets incremented.

    Also, while you are there:

    Line 85 uses "!=" but it is good practice to always use "!==" or "===".

    This is because 42 != "42" will return false.

    and 42 == "42" will return true.

    42 !== "42" will return true

    and 42 === "42" will return false

    === and !== check the value and type of the variable.

    == and != will check the variables, changing the types until it finds a match.

    Finally:

        var count = 0;
        var targetObj = {};
    

    Should be:

        var count = 0,
            targetObj = {};
    
  • Scott Downe

    Scott Downe February 28th, 2011 @ 11:35 AM

    • Assigned user changed from “Scott Downe” to “Min”
  • Min

    Min February 28th, 2011 @ 12:41 PM

    Ok, thanks for the tips. I fixed those syntax errors.
    About comma, yes, I didn't notice if I go back and forth it keeps incrementing, though if it's just one person, it won't put the comma. But anyways, seems that I need to change the logic.
    But I need some picture of how it really works, so I have couple of questions:
    1- Is there a way that I can debug frame by frame? I mean I can see in each frame what is the value of each element?
    2- I'm trying to find out if there is a way to know how many options object are being passed to the setup(). What I'm thinking is to add comma to the object in _setup so whenever start() is being called it won't increment it. Does it sound a better solution or not?

  • Scott Downe

    Scott Downe March 1st, 2011 @ 10:12 AM

    Min, setup is called uniquely per track added, so options sent into setup is the same object passed into start and end. Saying that, I don't think that is the way you need to go.

    You might want to have a look at this: https://github.com/annasob/popcorn-js/blob/0.1/popcorn.js#L356

    It is old code, so I think it can probably be improved, but the idea is use a class that can hold names, and return them as a string with commas.

    You would make an object out of this class per target.

    If you want to come by the office at any point to ask me questions feel free to do that.

  • Min

    Min March 1st, 2011 @ 09:24 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Ok, Scott, I changed back to the old version of the code (the one on 0.4 branch).
    The problem with the new code was adding the comma to string, which we didn't want to happen since we didn't want to attach it to the person's name(As far as I understood). So, I switched back to the old code which is creating an array on the fly on start().
    In new code, I was just trying to have everything ready in setup() and then just in start() bring them one by one.
    Which didn't work...

    So, I just made a little change to my tagthisperson branch (just added pipe instead of comma, in case you want to double check).
    However I created the new branch tagthispersonNew, to put the 0.4 version of the code.
    If the second one is ok with you, please let me know, so I update the unit test files.

    Unfortunately, I didn't come to school this week, otherwise asking in person would be the best way.

  • Scott Downe

    Scott Downe March 2nd, 2011 @ 09:51 AM

    • State changed from “peer-review-requested” to “super-review-requested”

    Right, it is study week.

    Anyway, good work.

    And thanks for cleaning up whitespace that was there before you got there.

    We should probably make unit tests for urls, but I also noticed we don't have unit tests for images either, so I think a new ticket to do both in one is in order.

    current unit tests pass.

    Semantic demo is intact.

    linting is already broken.

    Doesn't touch anything else.

  • Min

    Min March 2nd, 2011 @ 12:53 PM

    Well, thanks for being patient ;)

    Updated the unit tests @ tagthispersonNew

  • annasob

    annasob March 2nd, 2011 @ 05:22 PM

    • State changed from “super-review-requested” to “review-needs-work”

    I pulled the branch an the unit tests do not seem to work on FF i tried Beta and 3.6. Minoo please look into this. Also you did not add href in the comments at the top of the popcorn.tagthisperson.js file.

    Sorry

  • Min

    Min March 2nd, 2011 @ 09:40 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    I guess I replaced a push. So I just recreated the branch tagthispersonNew

    Sorry about that.

  • annasob

    annasob March 4th, 2011 @ 05:17 PM

    • State changed from “peer-review-requested” to “staged”

    Minoo this should have been set back to SR and not PR.

    I looked it over and everything looks good. Tests and Lint passes. SR+

    Staged in annasob/popcorn-js commit

  • Min
  • annasob

    annasob March 21st, 2011 @ 02:47 PM

    • State changed from “staged” to “checked-in”
  • annasob

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages