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

Add get_thumbs templatetag. #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ctbarna
Copy link
Contributor

@ctbarna ctbarna commented Apr 3, 2015

No description provided.

@ctbarna
Copy link
Contributor Author

ctbarna commented Apr 3, 2015

If we decide this is the way to go, I can write tests for it. In particular for prefetch things.

def get_thumbs(image):
def _serialize_thumb(thumb):
return {
"url": Image.get_file_for_size(image, thumb.name).url,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be related_object.get_image_url(thumb.name),?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the source for get_image_url:

    def get_image_url(self, size_name='original', tmp=False):
        converted = Image.get_file_for_size(self.image, size_name, tmp=tmp)
        return getattr(converted, 'url', None) or u''

It's doing the same thing as you are, but with a getattr that falls back to empty string. (I don't know if that would ever make a real world difference, but it's one less thing to blow up.) ISTM it's cleaner to use the method that gives you the url you want instead of doing it yourself and maybe having differences in how your code is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serious bikeshedding for working code that uses a public API.

@whatisjasongoldstein
Copy link
Contributor

I'm fine with this (and want One True Image Tag in the end). The only caveat is that some crop sizes have dashes in them and can't appear in the template. We'll want to migrate these to underscores, which is a good idea anyway.

(I will now await the obligatory snarky Jinja comment.)

@ctbarna
Copy link
Contributor Author

ctbarna commented Apr 3, 2015

I think this is the type of API that we want to move toward eventually but I'm not entirely convinced the future is in the templatetag. @joshmaker envisions a day where the API is something like article.lead_image.thumbs.300.url.

@whatisjasongoldstein
Copy link
Contributor

Agreed, if that's possible. It'd be nice to use the API the same way in views and templates, for setting the og:image, etc.

@fdintino
Copy link
Member

fdintino commented Jun 5, 2015

@ctbarna has this been abandoned in favor of putting the templatetag elsewhere? Or are we still planning to revisit this approach?

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.

4 participants