Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(233)

Issue 29572657: Issue 5787 - Add Topics Accordion to Article Template on help.eyeo.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by ire
Modified:
1 year, 11 months ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 8

Patch Set 2 : Addressed final NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -34 lines) Patch
A includes/product-topics-accordion.tmpl View 1 1 chunk +24 lines, -0 lines 0 comments Download
M static/js/main.js View 1 1 chunk +3 lines, -3 lines 0 comments Download
M templates/article.tmpl View 1 chunk +1 line, -31 lines 0 comments Download

Messages

Total messages: 4
ire
1 year, 11 months ago (2017-10-10 12:27:33 UTC) #1
ire
Ready for review
1 year, 11 months ago (2017-10-10 12:27:59 UTC) #2
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-topics-accordion.tmpl File includes/product-topics-accordion.tmpl (right): https://codereview.adblockplus.org/29572657/diff/29572658/includes/product-topics-accordion.tmpl#newcode3 includes/product-topics-accordion.tmpl:3: {% set articles = get_pages_metadata({ "product_id": ...
1 year, 11 months ago (2017-10-11 15:09:48 UTC) #3
ire
1 year, 11 months ago (2017-10-11 17:01:41 UTC) #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5