search

zurb / orbit

454 stars 96 forks source link

Fixed bug in caption handling + added a way to have slides w/ caption inside a link #17

Closed bpizzi closed 13 years ago

bpizzi commented 13 years ago

Hey guys,

There was a little nasty bug : you looked for slides.eq(activeSlide).data("caption") wheras your doc is telling us that a tag data-caption was where you stored the ID of the caption. So you surely meant something like slides.eq(activeSlide).attr("data-caption"), no ? In the meantime, I didn't dug that deep, and your examples do work, so maybe the real problem is little more complicated than that. Anyway, I fixed my invisibles captions with that workaround, so here it is.

And, bonus : I added a way to have slides(+caption) inside a link. I would have liked to have my slides inside a , like this ; wasn't possible until there. So there it is :)

mkelly12 commented 13 years ago

What version of jQuery are you using? The .data() method is automatically loaded with HTML5 data attributes as of jQuery 1.4.3.

If we want to add support for older versions of jQuery we should use .attr() everywhere instead of .data() for reading data attributes from elements.

code-poel commented 13 years ago

Was this issue fixed with the compressed version? I was still having issues until I made forced my application to use the uncompressed version.

mkelly12 commented 13 years ago

Thanks for the commit bpizzi, it's a bad practice to use $.fn.data() in plugins, since some people might be using an older version of jQuery. I've applied your fix in our new branch: 5e1f7a80da2f8121dd327074dceebdb9ae99ba2f

Sorry, but I was unable to merge your push request, because I significantly re-factored the code and it would not apply cleanly.

Regarding images in links, if you put the data-caption attribute on the anchor tag, it will work just fine.

bpizzi commented 13 years ago

Thanks for the updae @mkelly12 ;)

If we want to add support for older versions of jQuery we should use .attr() everywhere instead of .data() for reading data attributes from elements.

Indeed. But, if I'm not missing something obvious here, my PR was just about that : you were using data() and I changed it for attr().

Thanks for the commit bpizzi, it's a bad practice to use $.fn.data() in plugins, since some people might be using an older version of jQuery.

Yep, and I did used attr() in place of data() in my PR ;)

Sorry, but I was unable to merge your push request, because I significantly re-factored the code and it would not apply cleanly.

Absolutely np :)

Regarding images in links, if you put the data-caption attribute on the anchor tag, it will work just fine.

It was a long time ago, but if I remember well, that's exactly what wasn't working for me (maybe I was using and oooold jquery version :p)

mkelly12 commented 13 years ago

@bpizzi: You're right about using $.fn.attr() instead of $.fn.data(). It was a good pull request, and I would have accepted it had I not just refactored the code. I hope to get more pull requests from you in future once we get this factoring merged into master :)