Fixes #9 - Update cross-promotional section

includes/abb-notification.tmpl

Index: includes/abb-notification.tmpl
===================================================================
--- a/includes/abb-notification.tmpl
+++ b/includes/abb-notification.tmpl
@@ -1,7 +1,26 @@
-<div id="adblock-browser-notification" class="notice">

[juliandoucette, 2018/02/20 09:23:33]

NIT/Suggest: "notification" has new meaning now that there is a web notification
API. As a result, I think this name is confusing. I think that this could be
more accurately described as a "section" or an "ad" (honestly :D).

[ire, 2018/02/20 11:16:47]

I agree that notification is not ideal and I would like to change it.

One question though, if I change the string name "abb_notification" and the
corresponding file name, will that have an impact with translations?

If so, I would probably rather stick with the unideal name.

[juliandoucette, 2018/03/01 09:53:33]

You can change the include file name, ids, and classes. But you can't change the
string ids or files without losing translations. get_string accepts a string id
and locale file name (below). If you changed that, then you would have to change
it in all locales and on crowdin.

[ire, 2018/03/01 14:04:37]

In that case I would prefer to not have inconsistent naming within this
component, so I will keep it called "notification"

-  {{get_string("abb_notification", "abb-notification")}}
-  <span>
-    <img src="/img/abb-logo.png" alt="{{get_string("abb_logo_alt", "abb-notification")}}" />
-    <a href="https://adblockbrowser.org">{{get_string("abb_notification_link", "abb-notification")}}</a>
-  </span>
-</div>
+<section id="abb-notification" class="section">

[juliandoucette, 2018/02/20 09:23:33]

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 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.
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 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

+  <div class="container phablet-width content text-center">
+    <h2 class="h1">{{ get_string("abb_notification", "abb-notification") }}</h2>
+
+    <figure>
+      <img src="/img/abb-logo.png" alt="{{ get_string("abb_logo_alt", "abb-notification") }}" class="abb-logo">
+      <figcaption>
+        <h3>
+          <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 new window?

[juliandoucette, 2018/02/20 09:23:33]

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 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 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 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.

+            {{ get_string("abb_notification_link", "abb-notification") }}
+          </a>
+        </h3>
+        <p>{{ get_string("abb_notification_description", "abb-notification") }}</p>
+      </figcaption>
+    </figure>
+
+    <div id="abb-app-store-badges" class="row">
+      <a href="https://eyeo.to/adblockbrowser/ios/abp-website" title="{{ get_string("abb_ios_link_title", "abb-notification") }}" class="column one-half">
+        <img src="/img/apple-app-store-badge.png" srcset="/img/apple-app-store-badge.svg 2x" alt="{{ get_string("abb_ios_badge_alt", "abb-notification") }}">
+      </a>
+      <a href="https://eyeo.to/adblockbrowser/android/abp-website" title="{{ get_string("abb_android_link_title", "abb-notification") }}" class="column one-half">
+        <img src="/img/google-play-badge.png" srcset="/img/google-play-badge.svg 2x" alt="{{ get_string("abb_android_badge_alt", "abb-notification") }}">
+      </a>
+    </div>
+  </div>
+</section>