templates/article.tmpl - Issue 29556593: Issue 5690 - Create Article Template for help.eyeo.com

Delta Between Two Patch Sets: templates/article.tmpl

Issue 29556593: Issue 5690 - Create Article Template for help.eyeo.com (Closed) Base URL: https://hg.adblockplus.org/help.eyeo.com

Left Patch Set: Created Sept. 26, 2017, 9:12 a.m.

Right Patch Set: Addressed NITs Created Oct. 9, 2017, 7:46 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 {% extends "templates/default" %}	1 {% extends "templates/default" %}

2	2

3 {% set product = products[product_id] %}	3 {% set product = products[product_id] %}

4	4

5 {% block body %}	5 {% block body %}

6 [Breadcrumbs]	6 [Breadcrumbs]
ire 2017/09/26 09:16:43 Placeholder. I can remove it if it's preferred to Placeholder. I can remove it if it's preferred to just leave the place blank until the content is added in the relevant commits. (same for placeholders below) juliandoucette 2017/10/09 12:54:00 Acknowledged. Show quoted text On 2017/09/26 09:16:43, ire wrote: > Placeholder. I can remove it if it's preferred to just leave the place blank > until the content is added in the relevant commits. > > (same for placeholders below) Acknowledged.
7	7

8 <main id="main" class="container">	8 <main id="main" class="container clearfix">

9 <div class="row">	9 <article class="article card {{ product_id }}-card section column two-thirds">
ire 2017/09/26 09:16:43 This row class is removing the margins added by th This row class is removing the margins added by the container. I don't completely understand why the negative margins exist on the rows actually. Assuming there is a valid reason for having them, should we create a modifier class in website-defaults on the row that doesn't do this? juliandoucette 2017/10/09 12:54:00 This is expected. Show quoted text On 2017/09/26 09:16:43, ire wrote: > This row class is removing the margins added by the container. This is expected. Show quoted text > I don't completely understand why the negative margins exist on the rows actually. The negative margin exists to negate the first and last column padding. Components should handle horizontal padding and columns should handle horizontal margin. Imagine the following: div.container (padding: 0 1em) p (padding: 1em 0) .row (margin: 0 -1em) .column.one-half (padding: 0 1em) p .column.one-half p If .container and .column both have 1em padding then the .one-half p will have 2em padding at the beginning and end of the row (provided by the .container). .row negates the padding on the .container in-order-to align the .container p with the .column p(s). It works the same way with components. Text within components will align like this unless they are supposed to have more padding - which can/should be provided by the specific component. Show quoted text > class in website-defaults on the row that doesn't do this? We could do that. But .row and .column are not married (.column is styled, not .row > .column). You could just omit the .row if you don't want the negative margin. ire 2017/10/09 19:48:06 Ah okay. This makes sense (obviously :p). I will o Show quoted text On 2017/10/09 12:54:00, juliandoucette wrote: > On 2017/09/26 09:16:43, ire wrote: > > This row class is removing the margins added by the container. > > This is expected. > > > I don't completely understand why the negative margins exist on the rows > actually. > > The negative margin exists to negate the first and last column padding. > > Components should handle horizontal padding and columns should handle horizontal > margin. > > Imagine the following: > > div.container (padding: 0 1em) > p (padding: 1em 0) > .row (margin: 0 -1em) > .column.one-half (padding: 0 1em) > p > .column.one-half > p > > If .container and .column both have 1em padding then the .one-half p will have > 2em padding at the beginning and end of the row (provided by the .container). > .row negates the padding on the .container in-order-to align the .container p > with the .column p(s). > > It works the same way with components. Text within components will align like > this unless they are supposed to have more padding - which can/should be > provided by the specific component. > > > class in website-defaults on the row that doesn't do this? > > We could do that. But .row and .column are not married (.column is styled, not > .row > .column). You could just omit the .row if you don't want the negative > margin. Ah okay. This makes sense (obviously :p). I will omit it and add a .clearfix class to the #main instead, as that's all I really need for the .columns to function properly
10 <article class="article card {{ product_id }}-card section column two-thirds ">	10 <h1 class="article-heading">
juliandoucette 2017/10/09 12:53:59 NIT/Thinking-out-loud: We could add a product spec NIT/Thinking-out-loud: We could add a product specific class to the root element and then modify children instead. e.g. .abp a {color: red} .abp .card {border-top: red} ire 2017/10/09 19:48:06 This would work on some cases, but when we support Show quoted text On 2017/10/09 12:53:59, juliandoucette wrote: > NIT/Thinking-out-loud: We could add a product specific class to the root element > and then modify children instead. e.g. .abp a {color: red} .abp .card > {border-top: red} This would work on some cases, but when we support other products, there will be pages (e.g. the homepage) in which not all .cards will belong to one product. So I don't think we could always guarantee that a page will only have content for one product
11 <h1 class="article-heading">	11 <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcse t="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" \| tr anslate( product_id+"-logo-alt", "Image alt text") }}">

12 <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" src set="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" \| translate( product_id+"-logo-alt", "Image alt text") }}">
juliandoucette 2017/10/09 12:53:59 NIT: This doesn't seem to be vertically centred NIT: This doesn't seem to be vertically centred ire 2017/10/09 19:48:06 https://issues.adblockplus.org/ticket/5848 Show quoted text On 2017/10/09 12:53:59, juliandoucette wrote: > NIT: This doesn't seem to be vertically centred https://issues.adblockplus.org/ticket/5848 ire 2017/10/09 19:48:07 I will create a separate issue for this as it coul Show quoted text On 2017/10/09 12:53:59, juliandoucette wrote: > NIT: This doesn't seem to be vertically centred I will create a separate issue for this as it could apply to all .heading-icon images
13	12

14 {{ title \| translate( get_page_name(page) + "-title", "Article title") }}	13 {{ title \| translate( get_page_name(page) + "-title", "Article title") } }

15 </h1>	14 </h1>

16	15

17 {% if hide_browser_selector is not defined %}	16 {% if hide_browser_selector is not defined %}
juliandoucette 2017/10/09 12:53:59 Neat Neat ire 2017/10/09 19:48:06 Thanks :) Show quoted text On 2017/10/09 12:53:59, juliandoucette wrote: > Neat Thanks :)
18 <div class="article-browser-selector">	17 <div class="article-browser-selector">

19 [Browser selector]	18 [Browser selector]

20 </div>	19 </div>

21 {% endif %}	20 {% endif %}

22	21

23 <div class="article-body content">	22 <div class="article-body content">

24 {{ body \| safe }}	23 {{ body \| safe }}

25 </div>	24 </div>

26 </article>	25 </article>

27 <aside class="section column one-third">	26 <aside class="section column one-third">

28 [Topics Accordion]	27 [Topics Accordion]

29 </aside>	28 </aside>

30 </div>

31 </main>	29 </main>

32 {% endblock %}	30 {% endblock %}

LEFT	RIGHT