includes/abb-notification.tmpl - Issue 29690912: Fixes #9 - Update cross-promotional section

Delta Between Two Patch Sets: includes/abb-notification.tmpl

Issue 29690912: Fixes #9 - Update cross-promotional section (Closed) Base URL: https://hg.adblockplus.org/web.adblockplus.org

Left Patch Set: Created Feb. 6, 2018, 4:24 p.m.

Right Patch Set: Change background image to solid colour Created Feb. 28, 2018, 8:40 a.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 <section id="abb-notification" class="section">	1 <section id="abb-notification" class="section bg-secondary">
juliandoucette 2018/02/20 09:23:33 NIT/Suggest: This include only appears on one page NIT/Suggest: This include only appears on one page and does not use any template functionality AFAICT. For that reason, I think that it should be implemented in a page instead of an include, and/or in HTML format instead of Jinja2. Detail: The homepage is currently one big include IIRC. By "in a page" I mean "in that include" in this context. ire 2018/02/20 11:16:47 I'm a bit unsure about this. Since this only appea Show quoted text On 2018/02/20 09:23:33, juliandoucette wrote: > NIT/Suggest: This include only appears on one page and does not use any template > functionality AFAICT. For that reason, I think that it should be implemented in > a page instead of an include, and/or in HTML format instead of Jinja2. > > Detail: The homepage is currently one big include IIRC. By "in a page" I mean > "in that include" in this context. I'm a bit unsure about this. Since this only appears on one page, I am inclined to agree that it shouldn't be a separate include. However, the index file is currently so long that I think it's more readable to have things separated into different includes. I would prefer to keep it in this include for now, at least until the rest of the index page is cleaned up. Also, doesn't `get_string()` need to be inside a Jinja2 template? juliandoucette 2018/02/20 14:06:17 Acknowledged. Show quoted text On 2018/02/20 11:16:47, ire wrote: > I would prefer to keep it in this include for now, at least > until the rest of the index page is cleaned up. Acknowledged. Show quoted text > Also, doesn't `get_string()` need to be inside a Jinja2 template? Yes and no. {{ get_string(id, page) }} is the same as {{ id }} in a markdown or html file, provided the page passed to get_string is the current page. This implementation seems to be using a separate locale file, which makes sense when you consider the way that the existing homepage is implemented (each browser has it's own page and includes it's contents). However, we are going to change the way that this page is implemented (browser detection on the frontend instead of the backend), and our new implementation will not have this ~limitation (situation in which it makes sense to use a separate locale file). Therefore, we could keep this as-is and change it later. Or, we could change it now and not publish said change until the rest of the page is completed. Either way, we should avoid publishing translations until both of these issues are resolved to avoid duplication. ire 2018/02/21 08:53:16 Ack. If it is the case that this will change acros Show quoted text On 2018/02/20 14:06:17, juliandoucette wrote: > On 2018/02/20 11:16:47, ire wrote: > > I would prefer to keep it in this include for now, at least > > until the rest of the index page is cleaned up. > > Acknowledged. > > > Also, doesn't `get_string()` need to be inside a Jinja2 template? > > Yes and no. > > {{ get_string(id, page) }} > > is the same as > > {{ id }} > > in a markdown or html file, provided the page passed to get_string is the > current page. > > This implementation seems to be using a separate locale file, which makes sense > when you consider the way that the existing homepage is implemented (each > browser has it's own page and includes it's contents). However, we are going to > change the way that this page is implemented (browser detection on the frontend > instead of the backend), and our new implementation will not have this > ~limitation (situation in which it makes sense to use a separate locale file). > > Therefore, we could keep this as-is and change it later. Or, we could change it > now and not publish said change until the rest of the page is completed. Either > way, we should avoid publishing translations until both of these issues are > resolved to avoid duplication. Ack. If it is the case that this will change across the whole homepage, then I think we should do it altogether and keep this review to just the layout/design change. juliandoucette 2018/02/26 13:11:26 Agreed Show quoted text On 2018/02/21 08:53:16, ire wrote: > Ack. If it is the case that this will change across the whole homepage, then I > think we should do it altogether and keep this review to just the layout/design > change. Agreed	juliandoucette 2018/02/28 19:48:21 NIT: This isn't really a "notification" anymore IM NIT: This isn't really a "notification" anymore IMO (if it ever was anything resembling that). Please consider renaming it to something else e.g. "promotion". ire 2018/03/01 09:11:10 You already brought this up here - https://coderev Show quoted text On 2018/02/28 19:48:21, juliandoucette wrote: > NIT: This isn't really a "notification" anymore IMO (if it ever was anything > resembling that). Please consider renaming it to something else e.g. > "promotion". You already brought this up here - https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi... I think you may have missed my response? juliandoucette 2018/03/01 09:53:33 Yes, thank you. I'll respond there. Show quoted text On 2018/03/01 09:11:10, ire wrote: > You already brought this up here - > https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi... > > I think you may have missed my response? Yes, thank you. I'll respond there.
2 <div class="container phablet-width content text-center">	2 <div class="container phablet-width content text-center">

3 <h2 class="h1">{{ get_string("abb_notification", "abb-notification") }}</h2>	3 <h2 class="h1">{{ get_string("abb_notification", "abb-notification") }}</h2>

4	4

5 <figure>	5 <figure>

6 <img src="/img/abb-logo.png" alt="{{ get_string("abb_logo_alt", "abb-notif ication") }}" class="abb-logo">	6 <img src="/img/abb-logo.png" alt="{{ get_string("abb_logo_alt", "abb-notif ication") }}" id="abb-logo">

7 <figcaption>	7 <figcaption>

8 <h3>	8 <h3>{{ get_string("abb_notification_link", "abb-notification") }}</h3>

9 <a href="https://eyeo.to/adblockbrowser/abp-website">
juliandoucette 2018/02/20 09:23:33 NIT: I suspect Jeen would like this to open in a n NIT: I suspect Jeen would like this to open in a new window? juliandoucette 2018/02/20 09:23:33 NIT/Detail: This link doesn't make any sense to me NIT/Detail: This link doesn't make any sense to me :/. It seems like more than this heading should be clickable if this heading is clickable. ire 2018/02/20 11:16:47 Would you like to bring this up with Jeen? As this Show quoted text On 2018/02/20 09:23:33, juliandoucette wrote: > NIT/Detail: This link doesn't make any sense to me :/. It seems like more than > this heading should be clickable if this heading is clickable. Would you like to bring this up with Jeen? As this is how it is in the spec. For the record I do sort of get where you're coming from, but I don't think more should be clickable. I think either the heading alone or nothing at all. ire 2018/02/20 11:16:47 If we do this, wouldn't we have to have all the us Show quoted text On 2018/02/20 09:23:33, juliandoucette wrote: > NIT: I suspect Jeen would like this to open in a new window? If we do this, wouldn't we have to have all the usual things we do for links that open in a new window? juliandoucette 2018/02/20 14:06:17 Yes, an external link icon; I think :/ (Obviously Show quoted text On 2018/02/20 11:16:47, ire wrote: > If we do this, wouldn't we have to have all the usual things we do for links > that open in a new window? Yes, an external link icon; I think :/ (Obviously it would be strange to put an external link icon here. Again, I'll ask Jeen.) juliandoucette 2018/02/20 14:06:18 Acknowledged. I'll ask her. Show quoted text On 2018/02/20 11:16:47, ire wrote: > Would you like to bring this up with Jeen? As this is how it is in the spec. > > For the record I do sort of get where you're coming from, but I don't think > more should be clickable. I think either the heading alone or nothing at all. Acknowledged. I'll ask her.
10 {{ get_string("abb_notification_link", "abb-notification") }}

11 </a>

12 </h3>

13 <p>{{ get_string("abb_notification_description", "abb-notification") }}< /p>	9 <p>{{ get_string("abb_notification_description", "abb-notification") }}< /p>

14 </figcaption>	10 </figcaption>

15 </figure>	11 </figure>

16	12

17 <div id="abb-app-store-badges" class="row">	13 <div id="abb-app-store-badges" class="row">

18 <a href="https://eyeo.to/adblockbrowser/ios/abp-website" title="{{ get_str ing("abb_ios_link_title", "abb-notification") }}" class="column one-half">	14 <a href="https://eyeo.to/adblockbrowser/ios/abp-website" title="{{ get_str ing("abb_ios_link_title", "abb-notification") }}" class="column one-half">

19 <img src="/img/apple-app-store-badge.png" srcset="/img/apple-app-store-b adge.svg 2x" alt="{{ get_string("abb_ios_badge_alt", "abb-notification") }}">	15 <img src="/img/apple-app-store-badge.png" srcset="/img/apple-app-store-b adge.svg 2x" alt="{{ get_string("abb_ios_badge_alt", "abb-notification") }}">

20 </a>	16 </a>

21 <a href="https://eyeo.to/adblockbrowser/android/abp-website" title="{{ get _string("abb_android_link_title", "abb-notification") }}" class="column one-half ">	17 <a href="https://eyeo.to/adblockbrowser/android/abp-website" title="{{ get _string("abb_android_link_title", "abb-notification") }}" class="column one-half ">

22 <img src="/img/google-play-badge.png" srcset="/img/google-play-badge.svg 2x" alt="{{ get_string("abb_android_badge_alt", "abb-notification") }}">	18 <img src="/img/google-play-badge.png" srcset="/img/google-play-badge.svg 2x" alt="{{ get_string("abb_android_badge_alt", "abb-notification") }}">

23 </a>	19 </a>

24 </div>	20 </div>

25 </div>	21 </div>

26 </section>	22 </section>

LEFT	RIGHT