Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
implement naive self-service banners for python.org #1413
Conversation
|
||
@register.simple_tag | ||
def render_active_banner(psf_pages_only=True): | ||
if not psf_pages_only: |
berkerpeksag
Apr 12, 2019
Member
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()
ewdurbin
Apr 18, 2019
Author
Member
little trickier than that, since we accept either value for the PSF pages. so filtering on psf_pages_only makes general banners not show.
class BannerAdmin(admin.ModelAdmin): | ||
list_display = ("title", "active", "psf_pages_only") | ||
|
||
admin.site.register(Banner, BannerAdmin) |
berkerpeksag
Apr 12, 2019
Member
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): |
berkerpeksag
Apr 12, 2019
Member
Do you plan to squash these two migrations when design of the Banner
model is finished?
@@ -38,6 +39,8 @@ <h1 class="call-to-action">Download the latest version of Python</h1> | |||
{% block content %} | |||
<div class="row download-list-widget"> | |||
|
|||
{% render_active_banner False %} |
@@ -26,6 +27,8 @@ | |||
{% block breadcrumbs %} | |||
{% load sitetree %} | |||
|
|||
{% render_active_banner True %} |
berkerpeksag
Apr 12, 2019
Member
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.
ewdurbin
Apr 18, 2019
Author
Member
with suggestion from jaap3, this isn't necessary. I was just being explicit :)
|
||
|
||
@register.simple_tag | ||
def render_active_banner(psf_pages_only=True): |
jaap3
Apr 13, 2019
Contributor
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.
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() |
jaap3
Apr 13, 2019
Contributor
It would be nice to have a custom QuerySet
/Manager
that provides an active()
method (i.e. Banner.objects.active().first()
)
'title': banner.title, | ||
'link': banner.link, | ||
} | ||
return tmpl.render(ctx) |
jaap3
Apr 13, 2019
Contributor
render_to_string
does the same in less lines:
https://docs.djangoproject.com/en/2.2/topics/templates/#django.template.loader.render_to_string
Allows management of banners displayed across python.org via Django admin.