Add view partials for repeated content on post or topic writer #221

carofun · 2021-02-09T11:55:49Z

Purpose

If we want to override the way the last writer of a post or a topic is shown, the project forces us to repeat
our modification in multiple templates. We would like to make it easier.

Proposal

We introduce a partial that deals with the different usages.
It can be call with different options to take in charge the different cases of the project :

username_highlighted : if username has to be highlighted
creation_date : if we show the creation date
no_trans : if True we won't display the translation text By:

To ease the overload, we've added a block when we could. Two files only call once the new partial so we introduce a block written_by around it.
This way if we just want to change this in the template, we won't have to overload the whole file, just this block.
It's probably a case that will happen. For instance, in our personal use case, we will need to send an extra parameter to the partial.

ellmetha

Thanks for submitting this, I think it's fair to have a partial for this use case! Though this PR might need more a bit work in order to be merged (see my comments below).

ellmetha · 2021-03-03T01:42:56Z

machina/templates/machina/forum/forum_list.html

-        By: {{ poster_username }}
-        {% endblocktrans %}
-        {% endif %}
+        {% include "partials/posted_by.html" with poster=node.last_post.poster username=poster_username %}


{{ poster_username }} doesn't exist in the context. Shouldn't it be {{ node.last_post.username }} instead? Otherwise a blank string will be displayed for anonymous users. This question also applies to all the where the posted_by.html partial was inserted. 😉

Thank you for you feedback, and sorry about that, it is now fixed !

ellmetha · 2021-03-03T01:52:31Z

machina/templates/machina/partials/posted_by.html

+{% if display_label_by == None or display_label_by %}
+    {% trans "By:" %}
+{% endif %}
+{% if poster %}
+    {% include "partials/profile_link.html" %}
+{% else %}
+    {% if username_highlighted %}<b>{% endif %}{{ username }}{% if username_highlighted %}</b>{%endif%}
+{% endif %}


This block is not i18n-friendly: the "By:" string is inserted as a prefix whereas in the previous version of the code this was fully handled as a proper translation with variable substitutions.

Thank you for you feedback, I rewrote the partial and didn't cut any translation.

ellmetha · 2021-03-03T01:52:47Z

machina/templates/machina/partials/posted_by.html

+{% if creation_date %}
+    {% blocktrans trimmed%}
+        on {{ creation_date }}
+    {% endblocktrans %}
+{% endif %}


Same comment!

If we want to override the way the last writer of a post or a topic is shown, the project forces us to repeat our modification in multiple templates. We introduce a partials to simplify the overload.

ellmetha · 2021-03-14T13:41:14Z

machina/templates/machina/forum_conversation/topic_list.html

-                      By: {{ poster_username }} on {{ creation_date }}
-                    {% endblocktrans %}
-                  {% endif %}
+                  {% include "partials/posted_by.html" with poster=topic.poster creation_date=topic.created username=topic.first_post.username %}


This will result in a new SQL query for each topic because the topic.first_post is done systematically now.

ellmetha · 2021-03-14T13:45:31Z

machina/templates/machina/partials/posted_by.html

@@ -0,0 +1,33 @@
+{% load forum_member_tags %}


I think we could probably improve how this partial works so that it - kindof - keep working like things were working previously.

For example why not:

have a partial that either works from a single post variable or topic variable (depending on whether you want to display the poster info for the post or the topic itself) - this would allow to avoid passing poster, username and creation_date systematically in each partial use (which would be less error prone in my opinion)

then in order to control whether the creation date has to be displayed, you could use an extra show_creation_date variable (True / False)

no_trans would be replaced by something more explicit like show_by_prefix

username_highlighted would be replaced by highlight_username (True / False) to improve consistency

What do you think?

Yes, definitely, I think it's way simpler this way. I'll work around it and suggest it as a fix.
thank you for you feedback

I've commited with a fixup, I had forgotten to add a file with the search in the previous commit which was another case.
I don't know what to think of this new version. I had to make more cases to be able to read username as it is not specified anymore.
Tell me please what you think of it.

ellmetha · 2021-05-29T21:29:37Z

machina/templates/machina/partials/posted_by.html

+{% if is_search %}
+    {% if item.poster %}
+        {% url 'forum_member:profile' item.poster as poster_url %}
+        {% blocktrans trimmed with username=item.poster_name creation_date=item.created %}
+            By: <a href="{{ poster_url }}">{{ username }}</a> on {{ creation_date }}
+        {% endblocktrans %}
+    {% else %}
+        {% blocktrans trimmed with poster_username=item.poster_name creation_date=item.created %}
+        By: {{ poster_username }} on {{ creation_date }}
+        {% endblocktrans %}
+    {% endif %}
+{% else %}


The fact that we need to pass is_search=True in order to render this partial templates makes me think that it's probably equivalent to simply let the initial search template untouched. Otherwise would it be possible to make this partial template generic enough so that we don't have to use this is_search variable? If the partial is generic enough, it shouldn't make any assumption regarding the context in which it is used. 🤷‍♂️

Hi I had a second look at it, IMO, we should pass through as we did before (before the fixup), the username, the creation date and poster now that we have 3 cases : topics, posts and search, this way the partial will be generic.
Before I go for it, I prefer to have your feedback.
Thank you

We can try! Though I think the resulting partial should be as simple as possible and not contain too much logic.

carofun force-pushed the custom/partial_written_by branch 3 times, most recently from b06f81a to 60e0cb1 Compare February 9, 2021 13:19

ellmetha reviewed Mar 3, 2021

View reviewed changes

carofun force-pushed the custom/partial_written_by branch 4 times, most recently from afcae03 to 006824a Compare March 5, 2021 15:27

Add view partials for repeated content on post or topic writer

d35c029

If we want to override the way the last writer of a post or a topic is shown, the project forces us to repeat our modification in multiple templates. We introduce a partials to simplify the overload.

carofun force-pushed the custom/partial_written_by branch from 006824a to d35c029 Compare March 5, 2021 15:49

carofun requested a review from ellmetha March 5, 2021 15:59

ellmetha reviewed Mar 14, 2021

View reviewed changes

fixup! Add view partials for repeated content on post or topic writer

bb440c9

carofun requested a review from ellmetha March 19, 2021 15:27

ellmetha reviewed May 29, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add view partials for repeated content on post or topic writer #221

Add view partials for repeated content on post or topic writer #221

carofun commented Feb 9, 2021 •

edited

ellmetha left a comment

ellmetha Mar 3, 2021

carofun Mar 5, 2021

ellmetha Mar 3, 2021

carofun Mar 5, 2021

ellmetha Mar 3, 2021

ellmetha Mar 14, 2021

ellmetha Mar 14, 2021

carofun Mar 16, 2021

carofun Mar 19, 2021 •

edited

ellmetha May 29, 2021

carofun Jul 2, 2021

ellmetha Jul 10, 2021

Add view partials for repeated content on post or topic writer #221

Are you sure you want to change the base?

Add view partials for repeated content on post or topic writer #221

Conversation

carofun commented Feb 9, 2021 • edited

Purpose

Proposal

ellmetha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carofun Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carofun commented Feb 9, 2021 •

edited

carofun Mar 19, 2021 •

edited