Add support for wrapping image in anchor element #52

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants

Often these images are used as anchors, I'd like to include an automated method for generating the anchor that wraps the image.

peterbenoit added some commits Jan 25, 2013

Update index.html
Add data-href for use in the wrapping anchor
Update picturefill.js
Check for data-href and wrap the image

HA! I just added this to the current version I'm using. Didn't know anyone else was working on it. So therefore.. I second this motion 👍

Owner

scottjehl commented Apr 3, 2013

Thanks for the PR.

Could you explain the reason you can't currently wrap an anchor around the picturefill element?

That's how it would work with a native picture element one day, I'd expect, and I think keeping features relatively in-sync with that planning is a good idea.

Well it's not semantically correct to wrap a div with an a element. And it's less semantic to wrap multiple docs with a span. And then that span with an a. When the picture spec comes out, picture and its derivative source tags will all semantically fall within the bounds that allow them to be wrapped in an a.

Furthermore, I use Drupal for content management and tinymce as my main editor. Because the behavior of the editor is controlled by semantics, it won't allow the data-picture div to be wrapped by an a tag. Therefore I must rely on data-href.

Just because we're using a non HTML5 fallback to mimic HTML5, doesn't mean we still can't be semantic, right? Bend the rules, don't break them and all that.

I know that this method fails strict XHTML validation anyway you look at it. I really just want to be able to embed responsive images in body text using tinymce.

Thanks!

Sent from my iPhone

On Apr 3, 2013, at 2:10 PM, Scott Jehl notifications@github.com wrote:

Thanks for the PR.

Could you explain the reason you can't currently wrap an anchor around the picturefill element?

That's how it would work with a native picture element one day, I'd expect, and I think keeping features relatively in-sync with that planning is a good idea.


Reply to this email directly or view it on GitHub.

zigotica commented Apr 3, 2013

a can wrap anything in HTML5
the problem i foubd (thats why i forked picturefill) is when you need to add a picture inside a P, some browsers will force the div.picture outside the P, potentially breaking layout/design. span was a much better choice imho

Owner

scottjehl commented Apr 3, 2013

Agree about HTML5 and lenience on wrapping elements - strictness from HTML4 was relaxed in that case.

Anyway, I'm generally in favor of moving to span, or allowing any old markup with the expected attributes, which would solve this issue. The reason for moving to span would be that img is an inline styled element by default, and it'd be nice if picturefill matched that without additional CSS.

Semi unrelated but I'm not familiar with browsers moving divs outside of the p element - any particulars on that?

On Apr 3, 2013, at 1:34 PM, Sergi Meseguer notifications@github.com wrote:

a can wrap anything in HTML5
the problem i foubd (thats why i forked picturefill) is when you need to add a picture inside a P, some browsers will force the div.picture outside the P, potentially breaking layout/design. span was a much better choice imho


Reply to this email directly or view it on GitHub.

zigotica commented Apr 3, 2013

not sure whether it qas ff/safari or fckeditor, the point is somehow div.picture was SOMETIMES out of P's

mattheu commented May 5, 2013

I have also run into this issue. Chrome (26.0.1410.65) and Firefox (20) will move the <div data-picture> outside of the <p>

I have had issues when a <div data-picture> is contained within an <a> within a <p>. The <img> is then moved outside of the <a>, breaking the link.

This is fixed by changing picturefill to use spans instead of divs

This is fixed with the new span markup. I believe it can be closed.

Owner

scottjehl commented Jun 25, 2013

closes #52

On Jun 25, 2013, at 11:41 AM, Rob Wierzbowski notifications@github.com wrote:

This is fixed with the new span markup. I believe it can be closed.


Reply to this email directly or view it on GitHub.

@scottjehl scottjehl closed this Jun 26, 2013

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