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

2461 dynamic image url redux #4856

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zerolab
Copy link
Contributor

zerolab commented Oct 27, 2018

This continues #2461 with a a few tweaks:

  • rebased on and updated for latest master
  • added regular template tag tests
  • made the serving view an optional parameter
  • updated docs
try:
return generate_image_url(image, filter_spec, viewname)
except NoReverseMatch:
raise template.TemplateSyntaxError(

This comment has been minimized.

@m1kola

m1kola Oct 27, 2018

Member

Hm. I'm not sure if TemplateSyntaxError is a good option. Syntax of the template is correct, but the configuration is not.

Maybe we should use Django's ImproperlyConfigured? We use it widely in Wagtail.

Other than this, looks good

@zerolab zerolab force-pushed the zerolab:2461-dynamic-image-url-redux branch from 9d3ccd1 to 6fdf50a Oct 28, 2018

@m1kola

m1kola approved these changes Oct 28, 2018

Copy link
Member

m1kola left a comment

LGTM. Thanks, Dan

@m1kola m1kola requested a review from gasman Oct 28, 2018

@gasman
Copy link
Collaborator

gasman left a comment

Just one nitpick - other than that, looks good to me :-)

Show resolved Hide resolved wagtail/images/templatetags/wagtailimages_tags.py Outdated
Link to the stable version of docs when the image serve view is not c…
…onfigured [skip ci]

Co-Authored-By: zerolab <dan@zerolab.org>
@gasman

gasman approved these changes Oct 28, 2018

@m1kola m1kola added this to the 2.4 milestone Oct 28, 2018

@m1kola

This comment has been minimized.

Copy link
Member

m1kola commented Oct 28, 2018

Rebased and merged in 515aa61. Release notes added in 604bc6c.

Thanks @zerolab and @ychab

UPD: Sorry, I didn't add your names into the release notes. Fixed in 4132115

@m1kola m1kola closed this Oct 28, 2018

m1kola added a commit that referenced this pull request Oct 28, 2018

@jjanssen

This comment has been minimized.

Copy link
Member

jjanssen commented Oct 28, 2018

Possible next step for implementation #2364

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