Issue 5511 - Create Product Help Home Template for help.eyeo.com

templates/product-home.tmpl

 {% extends "templates/default" %}
 
+{% set product = products[product_id] %}
+
 {% block body %}
-<main id="main" class="wrapper">
-  <h1 class="h1 section ta-center">

[juliandoucette, 2017/09/12 13:21:58]

NIT: I think it's confusing to add a class named "section" to a header because a
header logically belongs to a "section". I think that this indicates that this
class should be named less specifically or this  heading should belong to a
component that styles headings differently. I'm leaning towards the latter e.g.
.product-heading

[ire, 2017/09/19 10:27:21]

Okay, I like the .product-heading way. Done.

-    <img class="heading-icon" src="/img/png/logo-{{product_id}}.png" srcset="/img/svg/logo-{{product_id}}.svg 2x" alt="{{ products[product_id].full_name+" Logo" | translate( product_id+"-logo-alt", "Image alt text") }}">

[juliandoucette, 2017/09/12 13:21:59]

NIT: I prefer string interpolation in python e.g. '%s-logo-alt' % product_id

[ire, 2017/09/19 10:27:22]

I actually find it more confusing, particularly because we're not using it
everywhere (e.h. the image urls)

[juliandoucette, 2017/09/22 13:29:36]

Acknowledged.

Please fix the spacing then.

+<main id="main" class="container">
+  <h1 class="product-heading">
+    <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate( product_id+"-logo-alt", "Image alt text") }}">
 
-    {{ products[product_id].full_name + " Help Center" | translate(product_id+"-help-home-title", "Page title") }}

[juliandoucette, 2017/09/12 13:21:58]

NIT: I suggest you {% set product = products[product_id] %} so that you can {{
product.full_name }} (or equivalent) 

[ire, 2017/09/19 10:27:20]

Good idea. Done.

+    {{ product.full_name + " Help Center" | translate(product_id+"-help-home-title", "Page title") }}
   </h1>
 
   {% set popular_topics = get_pages_metadata({ "product_id": product_id, "template": "article", "popular": "true" }) %}
   {% if popular_topics %}
-  <section class="card {{product_id}}-card section">
-    <h2 class="h4 card-heading">

[juliandoucette, 2017/09/12 13:21:59]

NIT: If all cards of the same type have the same font-size then it doesn't make
sense to set their font-size using a separate class?

[ire, 2017/09/19 10:27:22]

Done.

+  <section class="card {{ product_id }}-card section">
+    <h2 class="card-heading">
       <img class="heading-icon" src="/img/png/popular-icon.png" srcset="/img/svg/popular-icon.svg 2x" alt="{{ "Popular Icon" | translate("popular-icon-alt", "Image alt text") }}">
 
       {{ "Popular Topics" | translate("popular-topics-title", "Section title") }}
     </h2>
 
     <ul class="underlined-list row">

[juliandoucette, 2017/09/12 13:21:59]

Note: Clever use of row and columns here :)

[ire, 2017/09/19 10:27:20]

Thanks :)

       {% for article in popular_topics %}
       <li class="column one-half">
-        <a href="{{article["page"]}}">

[juliandoucette, 2017/09/12 13:21:59]

NIT: Inconsistent use of space in {{ }}

-          {{ article.title | translate( get_page_slug( article["page"]) + "-title", "Article title") }}
+        <a href="{{ article["page"] }}">
+          {{ article.title | translate( get_page_name( article["page"]) + "-title", "Article title") }}
         </a>
       </li>
       {% endfor %}
     </ul>
   </section>
   {% endif %}
 
   <div class="row">
-    {% for category in products[product_id].help_categories %}
+    {% for category in product.help_categories %}
     {% set articles = get_pages_metadata({ "product_id": product_id, "template": "article", "category": category.name }) %}

[juliandoucette, 2017/09/12 13:21:58]

NIT: I think that it's unnecessary to test against the template key/value here?

[ire, 2017/09/19 10:27:22]

Done.

     {% if articles and category.name != 'Popular Topics' %}
     <section class="section column one-half">

[juliandoucette, 2017/09/12 13:21:58]

Note: If you agree with my comment above about manually setting the font-size of
a card then it may logically follow to componentize this section. This section
is basically the inside of a card like the one above. Which suggests that we
could use the same single class for both.

[ire, 2017/09/19 10:27:20]

I agree about the font-size, but in the case it is a bit unclear where the
"component" should go. Because of where I am applying the columns, I can't use
the same structure for both sections.

[juliandoucette, 2017/09/22 13:29:36]

Acknowledged.

       <h2 class="h4">
-        <img class="heading-icon" src="/img/png/{{category.icon}}.png" srcset="/img/svg/{{category.icon}}.svg 2x" alt="{{ category.name+" Icon" | translate(category.icon+"-alt", "Image alt text") }}">
+        <img class="heading-icon" src="/img/png/{{ category.icon }}.png" srcset="/img/svg/{{ category.icon }}.svg 2x" alt="{{ category.name+" Icon" | translate(category.icon+"-alt", "Image alt text") }}">
 
-        {{category.name | translate( category.slug + "-category-title", "Category title")}}
+        {{ category.name | translate( category.slug + "-category-title", "Category title") }}
       </h2>
 
       <ul class="underlined-list">
         {% for article in articles %}
         <li>
-          <a href="{{article["page"]}}">
-            {{ article.title | translate( get_page_slug( article["page"]) + "-title", "Article title") }}
+          <a href="{{ article["page"] }}">
+            {{ article.title | translate( get_page_name( article["page"]) + "-title", "Article title") }}
           </a>
         </li>
         {% endfor %}
       </ul>
     </section>
     {% endif %}
     {% endfor %}
   </div>
 
 </main>
 {% endblock %}