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

Jinja version of bakerydemo #452

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

Jinja version of bakerydemo #452

wants to merge 1 commit into from

Conversation

thibaudcolas
Copy link
Member

@thibaudcolas thibaudcolas commented Oct 2, 2023

Targets a separate Jinja branch, which will remain fully separate from main. I thought I would open this as a pull request for feedback and also so there’s a good record of exactly what is needed to get a site like bakerydemo converted to Jinja. @laymonage and I discussed keeping the separate jinjademo branch for future compatibility testing with Jinja, though we’d need further consensus from other maintainers on whether this is worthwhile or not.

This does not include an initial commit moving all templates from templates/ to jinja2/.

Implementation notes:

  • I chose to not use django-jinja to avoid adding dependencies unless strictly necessary.
  • I went for the "least changes possible" version of the Jinja implementation to make it easier to sync this Jinja version with main in the future.

Because of those two choices, there are quite a few things in there that you’d probably not do this way in a Jinja project where the focus is reducing tech debt – but probably make sense with a focus on easy maintenance for us as a test site.


from wagtail.contrib.search_promotions.templatetags.wagtailsearchpromotions_tags import (
get_search_promotions,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only bakerydemo feature for which we don’t have Jinja support directly in Wagtail.

{% endblock %}
</title>
<meta name="description" content="{% if page.search_description %}{{ page.search_description }}{% endif %}">
<meta name="description" content="{{ page.search_description if page and page.search_description }}">
Copy link
Member Author

@thibaudcolas thibaudcolas Oct 2, 2023

Choose a reason for hiding this comment

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

This isn’t good in the base bakerydemo as views that aren’t page will have the meta tag with an empty value. Resolved in #453.

@@ -19,7 +17,7 @@ <h1 class="index-header__title index-header__title--blog">{{ page.title }}</h1>
{% endif %}
{% if page.date_published %}
<div class="blog__published">
{{ page.date_published }}
{{ page.date_published|date("DATE_FORMAT") }}
Copy link
Member Author

Choose a reason for hiding this comment

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

I’m surprised Jinja doesn’t do this by default, perhaps there’s a way with configuration.

<div class="picture-card__contents">
<p class="picture-card__title">{{ img.title }}</p>
</div>
</figure>
</div>
{{ image.title }}
Copy link
Member Author

@thibaudcolas thibaudcolas Oct 2, 2023

Choose a reason for hiding this comment

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

This looks odd and doesn’t seem to do anything even in the base demo site. Resolved in #453 too.

{% for menuitem in menuitems %}
<li class="presentation {{ menuitem.title|lower|cut:" " }}{% if menuitem.active %} active{% endif %}{% if menuitem.show_dropdown %} has-submenu{% endif %}">
<li class="presentation {{ menuitem.title|lower|cut(" ") }}{{ "active" if menuitem.active }} {{ "has-submenu" if menuitem.show_dropdown }}">
Copy link
Member Author

Choose a reason for hiding this comment

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

This "active" class does nothing currently so I’m not entirely sure why we have it.

{% image self.image fill-600x338 loading="lazy" %}
<figcaption>{{ self.caption }} - {{ self.attribution }}</figcaption>
{{ image(value.image, "fill-600x338", loading="lazy") }}
<figcaption>{{ value.caption }} - {{ value.attribution }}</figcaption>
Copy link
Member Author

Choose a reason for hiding this comment

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

Those fields are optional, which looks a bit odd right now in the base site.

@thibaudcolas
Copy link
Member Author

@laymonage this should actually be ready to review now (but this is very low-priority).

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.

1 participant