#368 ✓ staged
db48x

imagemacro plugin

Reported by db48x | February 24th, 2011 @ 04:03 PM | in 0.5

We either extend the image plugin so that it can overlay text on its image, or write another plugin that inherits from it.

Comments and changes to this ticket

  • Min

    Min March 5th, 2011 @ 09:12 PM

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

    Min March 5th, 2011 @ 09:28 PM

    For this purpose, is it ok to change the <img> element to a <div> with background?

  • db48x
  • Min

    Min March 9th, 2011 @ 08:33 PM

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

    Branch imageText

    I added the overlaying text. Also modified the implementation so that images appear in a specific size and overlaying text is in a specified position on the image.
    Also modified the unit test.
    make lint: passed
    make lint-plugins: passed for image plugin

  • Rick

    Rick March 11th, 2011 @ 05:12 PM

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

    Scott Downe March 24th, 2011 @ 11:24 AM

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

    Scott Downe March 24th, 2011 @ 12:27 PM

    • State changed from “under-review” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “Min”

    I like the use of a div with a background image, but

    I don't like the default to 200px x 200px. What you should do is make the height and widths of the created div, link and second div to 100%, and let the container div they supply hold the height and width.

  • Min

    Min March 24th, 2011 @ 09:27 PM

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

    Fixed the div size in branch imageText

    The only problem was that, as I understood, it looks like div doesn't like both width and height being set to 100%. So, I set the width to 100% and the height to 600px (which I thought might be a reasonable height for max, not sure though).
    Then to make the image.html page looks nicer ;) I just added a parent div to the .html file and set the size for that div to 400*400.

  • Scott Downe

    Scott Downe March 29th, 2011 @ 10:35 AM

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

    Yeah, I see the problems with height.

    I don't think setting it to 600px will be a appropriate solution.

    Few things that will improve this problem:

    background-size: 100% 100%; instead of background-size: 100% auto; on the background image.

    You also cano change:

      <div style="width:400px; height:400px">
        <div id="imagediv"></div>
      </div>
    

    to:

        <div style="width:400px; height:400px" id="imagediv"></div>
    

    We allow them to specify the target div so they can completely control the styling.

    This way, if they don't specify a height and width on the container, they get exactly what they asked for.

    I suspect though, this might not be a problem if the image wasn't a background image, and an image element. Try that maybe?

  • David Humphrey

    David Humphrey April 1st, 2011 @ 11:16 AM

    Minoo, how is this coming? Can we get this done for 0.5?

  • Min

    Min April 1st, 2011 @ 11:23 AM

    i'm actually working on it right now. I think I'll be done soon.

  • Min

    Min April 1st, 2011 @ 02:43 PM

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

    Changed the background in div to img element imageText

    The reason I used background was that all the researches I did on google, everybody were recommending to use it. img doesn't look very flexible with moving the text around. Like what I did in this patch, was using position:absolute; to be able to overlay the text on top of the image, but with the same reason I can't center the text on the image.
    So, for now, I used a fixed position, Is this fine?
    If everything is fine, I need to change the testcase.

    btw, I changed the previous solution as you recommended:

    background-size: 100% 100%
    

    didn't work with height and width set to 100%.

  • annasob

    annasob April 1st, 2011 @ 03:52 PM

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

    Im assigning to Scott so he can finish the PR he already started.

  • Scott Downe

    Scott Downe April 4th, 2011 @ 10:32 AM

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

    Scott Downe April 4th, 2011 @ 11:38 AM

    • State changed from “under-review” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “Min”

    The img vs background will solve the problem we found with background not having a usable height.

    Also, the text should be centered and overlayed on the image without a fixed position.

    I wrote up a quick hack to see what would work and what would not:

          _setup: function( options ) {
    
            options.link = document.createElement( 'a' );
            options.link.style.display = "none"; // display none by default
            options.link.style.position = "relative";
            options.link.style.height = "100%";
            options.link.style.width = "100%";
            if ( options.href ) {
              options.link.href = options.href;
            }
            options.link.target = "_blank";
            if ( document.getElementById( options.target ) ) {
              document.getElementById( options.target ).appendChild( options.link ); // add the widget's div to the target div
            }
    
            var img = document.createElement( 'img' );
            img.src = options.src;
            img.style.borderStyle = "none"; // borders look really bad, if someone wants it they can put it on their div target
            img.style.position = "absolute";
            img.style.height = "100%";
            img.style.width = "100%";
            
            var divText = document.createElement( 'div' );
            divTextStyle = {
                position: 'absolute',
                textAlign: "center",
                left: 0,
                right: 0,
                fontSize: 'large',
                color: 'black',
                fontWeight : 'bold',
                zIndex: '10',
            };
            for ( st in divTextStyle ) {
              divText.style[ st ] = divTextStyle[ st ];
            }
            divText.innerHTML = options.text;
            
            options.link.appendChild( divText );
            options.link.appendChild( img );
          },
    
          /**
           * @member image 
           * The start function will be executed when the currentTime 
           * of the video  reaches the start time provided by the 
           * options variable
           */
          start: function( event, options ) {
            options.link.style.display = "block";
          },
    

    The key here is position absolute inside a position relative, height and widths at 100%, and text-align center.

  • Min

    Min April 4th, 2011 @ 05:35 PM

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

    Ok, Scott, thanks for all your help and info.
    Here are the latest changes: imageText

    I didn't try and actually know anything about options.link.style.display = "block"; !
    That was a huge difference :)

    Just one more thing, I've attached to screenshots with and without these 2 lines:

       options.link.style.width = "100%";
       img.style.width = "100%";
    

    Looks like these two styles settings deform the width of image, and it wants to follow the parent width.
    So, I took them out for now. The only problem here is the textAlign follows the parent width and center based on that size.
    I tried different ways to control the location of text based on the image width but couldn't do it, since it's not appended to the document yet and I cant' get it's final width.

    Also, I changed the size of font based on parent height and also the location of text is changed. Hope these are fine.

  • annasob

    annasob April 7th, 2011 @ 01:28 PM

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

    Scott Downe April 13th, 2011 @ 10:53 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “Min”
    • Milestone order changed from “19” to “0”

    Sorry to do this to you again, but I am going to have to fail this.

    You can scrap all the width/height 100% nonsense.

    Somewhat an oversight on my behalf, mainly being the width and height 100% stuff was to solve a problem we only had because we were using background-image. Without using background-image, we can simply use the width and height of the image.

    Also, if text was left out, you would get the word "undefined" instead of nothing.

    Because this was partially my fault, and because of how nasty css can be, I have a proof of concept of what I think this should be:

          _setup: function( options ) {
    
            options.link = document.createElement( 'a' );
            options.link.style.position = "relative";
            
            var img = document.createElement( 'img' );
            img.src = options.src;
            img.style.position = "absolute";
    
            if ( options.href ) {
              options.link.href = options.href;
            }
            options.link.target = "_blank";
            if ( document.getElementById( options.target ) ) {
              document.getElementById( options.target ).appendChild( options.link ); // add the widget's div to the target div
            }
            img.style.borderStyle = "none"; // borders look really bad, if someone wants it they can put it on their div target
            
            var fontHeight = ( img.height / 12 ) + "px";
    
            var divText = document.createElement( 'div' );
            divTextStyle = {
                position: "absolute",
                width: img.width + "px",
                textAlign: "center",
                fontSize: fontHeight,
                color: "black",
                fontWeight : "bold",
                zIndex: "10",
            };
            for ( st in divTextStyle ) {
              divText.style[ st ] = divTextStyle[ st ];
            }
            divText.innerHTML = options.text || "";
            options.link.appendChild( divText );
            options.link.appendChild( img );
            divText.style.top = ( img.height / 2 ) - ( divText.offsetHeight / 2 ) + "px";
            options.link.style.display = "none"
          },
    
  • Min

    Min April 14th, 2011 @ 04:05 PM

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

    Updated the branch: imageText

    Applied your final suggestions on text position and ability of displaying more than one image at the same time.
    Updated the unit test.

    Thanks again Scott.

  • Scott Downe

    Scott Downe April 14th, 2011 @ 05:26 PM

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

    We got it.

  • annasob

    annasob April 15th, 2011 @ 07:10 PM

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

    Ok this is good but a couple of things:

    The documentation at the top of the page doesn't mention text being an option - please add. Also right now the unit test are using exec and your current tests are using setInterval. Please fix this, exec() is the right way to go. You can simply copy/paste from here and change test # 3 to use block instead of inline and add a text to options.

    Finally, fix the following lint errors:

    Linting ./plugins/image/popcorn.image.js
    
                zIndex: "10",
    
        Problem at line 77 character 25: Extra comma.
    
            for ( st in divTextStyle ) {
    
        Problem at line 79 character 15: Bad for in variable 'st'.
    
    2 Error(s) found.
    

    Set back to SR and assign to me when done :)

  • Min

    Min April 15th, 2011 @ 08:25 PM

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

    Done!
    Updated the branch: imageText (in two commits)

    Just after you mentioned about exec() in unit test, I remembered tagthisperson plugin had the same format of unit test (setInterval).
    Here is the branch and here is the ticket I worked on. Didn't know how necessary was it but just in case, I fixed that one too and commited.
    Was not sure, though to update that ticket or not, since it's 'checked-in'.

  • Scott Downe

    Scott Downe April 19th, 2011 @ 05:31 PM

          _setup: function( options ) {
            var st;
    
            options.link = document.createElement( 'a' );
            options.link.style.position = "relative";
            options.link.style.textDecoration = "none";
    
            var img = document.createElement( 'img' );
            img.addEventListener( "load", function() {
    
              img.style.borderStyle = "none"; // borders look really bad, if someone wants it they can put it on their div target
              
              if ( options.href ) {
                options.link.href = options.href;
              }
              options.link.target = "_blank";
              if ( document.getElementById( options.target ) ) {
                document.getElementById( options.target ).appendChild( options.link ); // add the widget's div to the target div
              }
              
              var fontHeight = ( img.height / 12 ) + "px";
              
              var divText = document.createElement( 'div' );
              divTextStyle = {
                  position: "relative",
                  width: img.width + "px",
                  textAlign: "center",
                  fontSize: fontHeight,
                  color: "black",
                  fontWeight : "bold",
                  zIndex: "10"
              };
              for ( st in divTextStyle ) {
                divText.style[ st ] = divTextStyle[ st ];
              }
              
              divText.innerHTML = options.text || "";
              options.link.appendChild( divText );
              options.link.appendChild( img );
              divText.style.top = ( img.height / 2 ) - ( divText.offsetHeight / 2 ) + "px"; 
              options.link.style.display = "none";
            }, false );
            img.src = options.src;
          },
    
  • Min

    Min April 19th, 2011 @ 09:00 PM

    Added the eventListener() to image. Branch imageText
    I was just wondering why the order of the images change when you refresh.
    Even if I enter an earlier start time for drumbeat, it appears after webmademovies image.

  • Scott Downe

    Scott Downe April 20th, 2011 @ 12:06 PM

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

    Min, the order they display must be dependent on the order they are loaded in. We now wait before the image is loaded before we add it to the dom, so whichever is loaded first gets added first.

    I wouldn't worry about that though, because image's css should be handled by the target divs, which would control the placement. If we decide we want to control that as well, we can premake target divs before the image loading starts, then place the loaded image in that target, which imo is out of scope of this ticket.

  • Scott Downe

    Scott Downe April 20th, 2011 @ 12:09 PM

    Filed #485 to deal with the location of the images.

    Min you are welcome to continue with this. The only reason I split it up is because I need to focus on 0.5's release, and that minor issue can be done in 0.6.

  • annasob

    annasob April 20th, 2011 @ 12:17 PM

    • State changed from “review-looks-good” to “staged”

    Staged in annasob/popcorn-js commit

  • Min

    Min April 20th, 2011 @ 04:21 PM

    Sure, I'll work on it.

  • Rick

    Rick May 8th, 2011 @ 12:45 PM

    You can reduce code and avoid any issues with the for-in by using:

    
    Popcorn.extend( divText.style, {
      position: "relative",
      width: img.width + "px",
      textAlign: "center",
      fontSize: fontHeight,
      color: "black",
      fontWeight : "bold",
      zIndex: 10
    });
    
  • Min

    Min May 10th, 2011 @ 11:19 PM

    Thanks for the tip, Rick.
    I'm just not sure if I'm allowed to change the code at this stage for this ticket. Should we file another ticket for it?

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

Attachments

Referenced by

Pages