implement naive self-service banners for python.org #1413
Conversation
banners/templatetags/banners.py
Outdated
|
|
||
| @register.simple_tag | ||
| def render_active_banner(psf_pages_only=True): | ||
| if not psf_pages_only: |
There was a problem hiding this comment.
If I understood this correctly, the following line would cover both if and else statements:
banner = Banner.objects.filter(active=True, psf_pages_only=psf_pages_only).first()There was a problem hiding this comment.
little trickier than that, since we accept either value for the PSF pages. so filtering on psf_pages_only makes general banners not show.
banners/admin.py
Outdated
| class BannerAdmin(admin.ModelAdmin): | ||
| list_display = ("title", "active", "psf_pages_only") | ||
|
|
||
| admin.site.register(Banner, BannerAdmin) |
There was a problem hiding this comment.
Style nit: We are trying to use the @admin.site.register() decorator version in new code.
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
Do you plan to squash these two migrations when design of the Banner model is finished?
templates/downloads/index.html
Outdated
| {% block content %} | ||
| <div class="row download-list-widget"> | ||
|
|
||
| {% render_active_banner False %} |
There was a problem hiding this comment.
Nit: Passing psf_pages_only=False would increase readability here.
There was a problem hiding this comment.
again, will be addressed with excellent suggestion from jaap3
templates/psf/default.html
Outdated
| {% block breadcrumbs %} | ||
| {% load sitetree %} | ||
|
|
||
| {% render_active_banner True %} |
There was a problem hiding this comment.
Nit: Did Django complained without passing an argument? I don't have a computer to test at the moment, but we might just use {% render_active_banner %} here.
There was a problem hiding this comment.
with suggestion from jaap3, this isn't necessary. I was just being explicit :)
banners/templatetags/banners.py
Outdated
|
|
||
|
|
||
| @register.simple_tag | ||
| def render_active_banner(psf_pages_only=True): |
There was a problem hiding this comment.
What about having two template tags render_active_banner and render_active_psf_banner instead of adding a boolean argument? This would also make it clearer what is going on in the template.
There was a problem hiding this comment.
great suggestion, thank you!
banners/templatetags/banners.py
Outdated
| if not psf_pages_only: | ||
| banner = Banner.objects.filter(active=True, psf_pages_only=psf_pages_only).first() | ||
| else: | ||
| banner = Banner.objects.filter(active=True).first() |
There was a problem hiding this comment.
It would be nice to have a custom QuerySet/Manager that provides an active() method (i.e. Banner.objects.active().first())
banners/templatetags/banners.py
Outdated
| 'title': banner.title, | ||
| 'link': banner.link, | ||
| } | ||
| return tmpl.render(ctx) |
There was a problem hiding this comment.
render_to_string does the same in less lines:
https://docs.djangoproject.com/en/2.2/topics/templates/#django.template.loader.render_to_string
3 participants
Allows management of banners displayed across python.org via Django admin.