Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29690912: Fixes #9 - Update cross-promotional section (Closed)

Created:
Feb. 6, 2018, 4:24 p.m. by ire
Modified:
March 1, 2018, 2:06 p.m.
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

Fixes #9 - Update cross-promotional section https://gitlab.com/eyeo/web.adblockplus.org/issues/9 Branch: index_page https://hg.adblockplus.org/web.adblockplus.org/shortlog/index_page

Patch Set 1 #

Total comments: 27

Patch Set 2 : Addressed comments #2 #

Patch Set 3 : Change background image to solid colour #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+302 lines, -52 lines) Patch
M includes/abb-notification.tmpl View 1 2 1 chunk +22 lines, -7 lines 3 comments Download
M includes/index.tmpl View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M locales/en/abb-notification.json View 1 chunk +19 lines, -5 lines 8 comments Download
M static/css/index.css View 1 2 2 chunks +30 lines, -6 lines 0 comments Download
M static/css/main.css View 1 2 3 chunks +9 lines, -30 lines 0 comments Download
M static/img/abb-logo.png View Binary file 0 comments Download
A static/img/apple-app-store-badge.png View Binary file 0 comments Download
A static/img/apple-app-store-badge.svg View 1 chunk +129 lines, -0 lines 0 comments Download
A static/img/google-play-badge.png View Binary file 0 comments Download
A static/img/google-play-badge.svg View 1 chunk +89 lines, -0 lines 0 comments Download
M templates/default.tmpl View 2 chunks +4 lines, -3 lines 0 comments Download

Messages

Total messages: 14
ire
Feb. 6, 2018, 4:25 p.m. (2018-02-06 16:25:03 UTC) #1
ire
@Manvel please can you have a look at this. Note that I'm working on the ...
Feb. 6, 2018, 4:31 p.m. (2018-02-06 16:31:14 UTC) #2
juliandoucette
Thanks Ire! Mostly NIT below :) https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl File includes/abb-notification.tmpl (left): https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl#oldcode1 includes/abb-notification.tmpl:1: <div id="adblock-browser-notification" class="notice"> ...
Feb. 20, 2018, 9:23 a.m. (2018-02-20 09:23:34 UTC) #3
ire
Thanks Julian! New patch uploaded https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl File includes/abb-notification.tmpl (left): https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl#oldcode1 includes/abb-notification.tmpl:1: <div id="adblock-browser-notification" class="notice"> On ...
Feb. 20, 2018, 11:16 a.m. (2018-02-20 11:16:48 UTC) #4
juliandoucette
Thanks Ire! https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl File includes/abb-notification.tmpl (right): https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl#newcode1 includes/abb-notification.tmpl:1: <section id="abb-notification" class="section"> On 2018/02/20 11:16:47, ire ...
Feb. 20, 2018, 2:06 p.m. (2018-02-20 14:06:18 UTC) #5
ire
Thanks Julian. Currently waiting for Jeen to update the spec/issue and provide me with updated ...
Feb. 21, 2018, 8:53 a.m. (2018-02-21 08:53:17 UTC) #6
juliandoucette
https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl File includes/abb-notification.tmpl (right): https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl#newcode1 includes/abb-notification.tmpl:1: <section id="abb-notification" class="section"> On 2018/02/21 08:53:16, ire wrote: > ...
Feb. 26, 2018, 1:11 p.m. (2018-02-26 13:11:27 UTC) #7
ire
New patch set
Feb. 28, 2018, 8:40 a.m. (2018-02-28 08:40:53 UTC) #8
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notification.tmpl File includes/abb-notification.tmpl (right): https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notification.tmpl#newcode1 includes/abb-notification.tmpl:1: <section id="abb-notification" class="section bg-secondary"> NIT: This ...
Feb. 28, 2018, 7:48 p.m. (2018-02-28 19:48:22 UTC) #9
ire
https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notification.tmpl File includes/abb-notification.tmpl (right): https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notification.tmpl#newcode1 includes/abb-notification.tmpl:1: <section id="abb-notification" class="section bg-secondary"> On 2018/02/28 19:48:21, juliandoucette wrote: ...
March 1, 2018, 9:11 a.m. (2018-03-01 09:11:11 UTC) #10
juliandoucette
https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl File includes/abb-notification.tmpl (left): https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notification.tmpl#oldcode1 includes/abb-notification.tmpl:1: <div id="adblock-browser-notification" class="notice"> On 2018/02/20 11:16:47, ire wrote: > ...
March 1, 2018, 9:53 a.m. (2018-03-01 09:53:33 UTC) #11
juliandoucette
https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-notification.json File locales/en/abb-notification.json (right): https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-notification.json#newcode18 locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the App Store" On ...
March 1, 2018, 9:58 a.m. (2018-03-01 09:58:15 UTC) #12
juliandoucette
Sorry about the triple posting :D https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-notification.json File locales/en/abb-notification.json (right): https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-notification.json#newcode18 locales/en/abb-notification.json:18: "message": "Download Adblock ...
March 1, 2018, 10:26 a.m. (2018-03-01 10:26:08 UTC) #13
ire
March 1, 2018, 2:04 p.m. (2018-03-01 14:04:37 UTC) #14
Thanks Julian! Pushing now

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (left):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <div id="adblock-browser-notification"
class="notice">
On 2018/03/01 09:53:33, juliandoucette wrote:
> On 2018/02/20 11:16:47, ire wrote:
> > On 2018/02/20 09:23:33, juliandoucette wrote:
> > > 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).
> > 
> > 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.
> 
> 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.

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

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
On 2018/03/01 10:26:07, juliandoucette wrote:
> On 2018/03/01 09:58:14, juliandoucette wrote:
> > Detail: There seems to be two separate alt texts provided in the spec. One
> under
> > "App store links" and another under "Assets".
> 
> I created https://gitlab.com/eyeo/specs/spec/issues/153
> 
> (Feel free to push and change this later if necessary.)

Ack. I will push now and change this later when the spec has been updated

Powered by Google App Engine
This is Rietveld