changed div's for span's, added -webkit- -moz- -o- prefixes #47

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants

I propose a change from divs to spans in order to being able to include picture inside paragraphs. Not just a matter of semantics, but some browsers will refuse to insert a div inside a p and will move the node outside.

Also, it took me some minutes to see that I needed to add an additional -webkit-min-device-pixel-ratio for each data-media, which is cumbersome since we are already using javascript to fill the gaps. I just added the following into the script:

if( media && media.indexOf("min-device-pixel-ratio") > -1 ) {
    var webkitmedia = media.replace("min-device-pixel-ratio", "-webkit-min-device-pixel-ratio");
    var mozmedia = media.replace("min-device-pixel-ratio", "-moz-min-device-pixel-ratio");
    var omedia = media.replace("min-device-pixel-ratio", "-o-min-device-pixel-ratio");
    media = webkitmedia +","+ mozmedia +","+ omedia +","+ media;
}

luksak commented Nov 15, 2012

Today I used Picturefill with this media query:

only screen and (-webkit-min-device-pixel-ratio: 2), only screen and (min-resolution: 2dppx)

It works on webkit based browsers and uses the standard min-resolution media query. I think this makes more sense. So, maybe we should rather encourage people to use the standard min-resolution media query and replace this one with vendor-prefixed min-device-pixel-ratio media queries?

Besides that, the -moz prefix is wrong. It should be min--moz-device-pixel-ratio.

Should I create a pull request for that?

+1 for changing to spans. I too have encountered the issue of browsers moving Picturefill divs outside their p parent. They are also moved outside of parent anchor elements in some cases. Using spans instead of divs would allow Picturefill to be used wherever an img would be used. CMSs like WordPress wrap user-inserted img tags in paragraph tags, so plugins that swap-in Picturefill markup will encounter the "DOM reordering" issues.

Since changing the script would break existing apps using it, it's better to just fork it and change the two lines in the js file that reference divs to spans, and modify your front end code as needed, to use spans. Pretty simple.

Side note, I've added classname and title support. I use this currently on my own sites, but should be in the picturefill master. #69

Owner

scottjehl commented Jun 6, 2013

spans are in master now, though I missed that this pull may have solved it similarly already. It was a small change so let me know if i should reopen. Thanks!

scottjehl closed this Jun 6, 2013

Should be "min--moz-device-pixel-ratio"

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