Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch image original either from data-original, or content or contentUrl attr #252

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PMPP
Copy link

@PMPP PMPP commented Mar 25, 2015

As microformats keep gaining traction, having attributes that describe better the page, and its information, becomes more and more relevant.

Many "objects" (movie, product, person, etc) can have a photo, which can be tag as itemprop="image". Normally if the element is an IMG the parser would refer to the SRC to know the content of that property. Since with lazy loading we lack that, other attributes can be used: content or contentUrl.

To avoid duplication of one could assume that a missing data-original implies the usage of content, or contentUrl to fetch the original image.

Thoughts?

@nicodemuz
Copy link

👍

@giver
Copy link

giver commented Mar 25, 2015

That is a good idea +:100:

@devzer01
Copy link

i agree, 👍

@PMPP
Copy link
Author

PMPP commented Apr 7, 2015

Rebuilt the min assets to solve conflict.

@tuupola
Copy link
Owner

tuupola commented Apr 7, 2015

Thanks. As mentioned in CONTRIBUTING should not include minified version in pull request. These are handled by the building process. No need to remove though. I can cherrypick around it if I decide to include this.

I like the idea of supporting microformats. For me the problem is the attribute names are hardcoded. Next year there will be a new microformat which uses different attribute name. More hardcoded logic needed. It would be better if the attribute name was configurable. This way user can use any arbitrary attribute name he or she chooses.

Since there already is data_attribute parameter this cannot be changed without breaking bc. One option would be to add attribute parameter which overrides data_attribute if the first one exists. Basically just resolve in constructor what the attribute name will be and use simple $.attr() in rest of the code.

$.attr() is anyway used already because it is faster than $.data() and does not cache the results.

@PMPP
Copy link
Author

PMPP commented Apr 7, 2015

Good point, I'll refactor once I get the opportunity and came for your appreciation. By that point I'll also remove the minified commit. Thanks for taking a look into this.

@PMPP
Copy link
Author

PMPP commented May 28, 2015

I've updated the PR, there for your consideration. Thanks!

@PMPP
Copy link
Author

PMPP commented Jul 30, 2015

Ping?

@nicodemuz
Copy link

@tuupola can this be merged in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants