Issue 5787 - Add Topics Accordion to Article Template on help.eyeo.com

Issue 5787 - Add Topics Accordion to Article Template on help.eyeo.com

[ire, 2017-10-10 12:27:59]

Ready for review

[juliandoucette, 2017-10-11 15:09:48]

LGTM + NITs

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
File includes/product-topics-accordion.tmpl (right):

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
includes/product-topics-accordion.tmpl:3: {% set articles = get_pages_metadata({
"product_id": product_id, "template": "article", "category": category.name }) %}
NIT: I think we indent after for

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
includes/product-topics-accordion.tmpl:5: <dt role="heading" aria-level="3"
class="accordion-heading">
NIT: I think we indent after if

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
includes/product-topics-accordion.tmpl:7: <img
src="/img/png/arrow-icon-secondary.png"
srcset="/img/svg/arrow-icon-secondary.svg 2x" alt="{{ "Toggle Section" |
translate("accordion-toggle-icon", "Image alt text") }}">
NIT/Suggest: "Toggle arrow"?  - more visually descriptive.

https://codereview.adblockplus.org/29572657/diff/29572658/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29572657/diff/29572658/static/js/main.js#n...
static/js/main.js:196: if (document.getElementById('product-topics-accordion'))
NIT: I would query once and [check, use] the result. But the difference is
negligible at this scale.

[ire, 2017-10-11 17:01:41]

Thanks

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
File includes/product-topics-accordion.tmpl (right):

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
includes/product-topics-accordion.tmpl:3: {% set articles = get_pages_metadata({
"product_id": product_id, "template": "article", "category": category.name }) %}
On 2017/10/11 15:09:48, juliandoucette wrote:
> NIT: I think we indent after for

Done.

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
includes/product-topics-accordion.tmpl:5: <dt role="heading" aria-level="3"
class="accordion-heading">
On 2017/10/11 15:09:48, juliandoucette wrote:
> NIT: I think we indent after if

Done.

https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-to...
includes/product-topics-accordion.tmpl:7: <img
src="/img/png/arrow-icon-secondary.png"
srcset="/img/svg/arrow-icon-secondary.svg 2x" alt="{{ "Toggle Section" |
translate("accordion-toggle-icon", "Image alt text") }}">
On 2017/10/11 15:09:48, juliandoucette wrote:
> NIT/Suggest: "Toggle arrow"?  - more visually descriptive.

Done.

https://codereview.adblockplus.org/29572657/diff/29572658/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29572657/diff/29572658/static/js/main.js#n...
static/js/main.js:196: if (document.getElementById('product-topics-accordion'))
On 2017/10/11 15:09:48, juliandoucette wrote:
> NIT: I would query once and [check, use] the result. But the difference is
> negligible at this scale.

I prefer your suggestion. Done.