#852 ✓ staged
Steven W

Create subtitle div if target specified but not in DOM tree

Reported by Steven W | November 29th, 2011 @ 10:51 PM | in 1.1 (closed)

I came across this while resolving #752.
If a subtitle target is specified, but it doesn't exist in the DOM tree, it silently errors. The subtitle is not displayed in this case.

Comments and changes to this ticket

  • Steven W

    Steven W November 29th, 2011 @ 11:18 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user cleared.

    Tested on FF 8, IE 9, Opera 11.52, Chrome 16, Safari 5.1.1
    Unit Tests added.

    Branch: https://github.com/stevenaw/popcorn-js/tree/t852
    Commit: https://github.com/stevenaw/popcorn-js/commit/t852

  • Scott Downe

    Scott Downe November 30th, 2011 @ 12:05 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user set to “Steven W”

    Intuitive solution to a hard problem.

    If you don't supply a target, the default div is created, with a guid for the id. Makes sense to me.

    If they supply a target, the subtitle uses that target. Also makes sense.

    But, what if the user wants to not supply a target div, and have the default one created, and later, get a reference to that element.

    This way, the user can supply the id to the container, and later reference it. Also, if the user attempts to supply a target, and it ends up going onto the video, it is pretty obvious the id is not correct.

    Intuitive and organic. I love it.

    I did notice you put these changes on top of what seems to be #752. Which is stated is blocked by this. Should probably land this one first, then land #752. r- because of that.

  • Steven W

    Steven W November 30th, 2011 @ 10:07 PM

    I must've branched from the wrong branch.
    Will land 752 first.

  • Steven W

    Steven W December 1st, 2011 @ 06:45 PM

    • Assigned user changed from “Steven W” to “Scott Downe”
    • State changed from “review-needs-work” to “peer-review-requested”

    And by land 752 I mean 852.
    I've separated out this fix from #752 and re-created the branch.
    PR request, same links as above.

  • Scott Downe

    Scott Downe December 12th, 2011 @ 01:21 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Scott Downe” to “cadecairos”

    Good stuff.

    Pass.

    Going to drop some info for super reviewer.

    Basically, if you supply an id to subtitle's target, it will use that div if it exists, and create one with that id if it does not exist. Also, if another track event tries to use that id, it will just use the created one, so there won't be any collisions.

  • cadecairos

    cadecairos December 12th, 2011 @ 05:28 PM

    • State changed from “super-review-requested” to “review-looks-good”

    Yup, this looks good.

    Subtitle unit tests passing on:

    Firefox 8,
    Chrome 15,
    Safari 5.1,
    Opera 11.52

    Passes lint.

  • cadecairos

    cadecairos December 12th, 2011 @ 05:32 PM

    • State changed from “review-looks-good” to “staged”
    • Assigned user changed from “cadecairos” to “Steven W”

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