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

Issue 29326238: Issue 3031 - Add Adblock Browser section to First Run Page (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 3 months ago by saroyanm
Modified:
4 years, 2 months ago
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue 3031 - Add Adblock Browser section to First Run Page

Patch Set 1 #

Total comments: 3

Patch Set 2 : Removed block for French locale #

Total comments: 24

Patch Set 3 : Changed the translatable message string IDs #

Patch Set 4 : Addressed Thomas comments #

Total comments: 8

Patch Set 5 : Make section consistent, horisontal separation and small fixes #

Patch Set 6 : Small fixes #

Total comments: 8

Patch Set 7 : top border fix for RTL #

Total comments: 2

Patch Set 8 : Addressed Thomas comments #

Total comments: 6

Patch Set 9 : Use border-start instead of rtl hack #

Total comments: 2

Patch Set 10 : Reverted back to table-cell #

Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -7 lines) Patch
M firstRun.html View 1 2 3 1 chunk +17 lines, -3 lines 0 comments Download
M firstRun.js View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M locale/en-US/firstRun.json View 1 2 2 chunks +10 lines, -1 line 0 comments Download
A skin/abb-logo.png View Binary file 0 comments Download
M skin/firstRun.css View 1 2 3 4 5 6 7 8 9 2 chunks +104 lines, -2 lines 0 comments Download

Messages

Total messages: 30
saroyanm
@Thomas can you please have a look ? https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css#newcode191 skin/firstRun.css:191: background: ...
4 years, 3 months ago (2015-09-10 09:05:08 UTC) #1
Sebastian Noack
Please note that this change shouldn't be visible if |#content:lang(fr)|.
4 years, 3 months ago (2015-09-10 09:16:17 UTC) #2
saroyanm
On 2015/09/10 09:16:17, Sebastian Noack wrote: > Please note that this change shouldn't be visible ...
4 years, 3 months ago (2015-09-10 09:56:25 UTC) #3
saroyanm
Changed the translatable message string IDs
4 years, 3 months ago (2015-09-11 09:05:22 UTC) #4
Thomas Greiner
https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html#newcode43 firstRun.html:43: <div id="acceptable-ads-block"> Detail: There's no need to change the ...
4 years, 3 months ago (2015-09-11 18:12:00 UTC) #5
saroyanm
Thomas can you please have a look on the comments I left in: https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css Just ...
4 years, 2 months ago (2015-09-15 09:42:38 UTC) #6
Thomas Greiner
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#newcode152 skin/firstRun.css:152: } On 2015/09/15 09:42:37, saroyanm wrote: > On 2015/09/11 ...
4 years, 2 months ago (2015-09-15 11:19:32 UTC) #7
saroyanm
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#newcode152 skin/firstRun.css:152: } On 2015/09/15 11:19:31, Thomas Greiner wrote: > On ...
4 years, 2 months ago (2015-09-15 12:43:32 UTC) #8
Thomas Greiner
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#newcode213 skin/firstRun.css:213: width: 1px; On 2015/09/15 12:43:31, saroyanm wrote: > On ...
4 years, 2 months ago (2015-09-15 14:15:50 UTC) #9
saroyanm
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#newcode213 skin/firstRun.css:213: width: 1px; On 2015/09/15 14:15:50, Thomas Greiner wrote: > ...
4 years, 2 months ago (2015-09-15 15:03:35 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#newcode242 skin/firstRun.css:242: html[dir="rtl"] #general > div:not(:first-child) The second selector is redundant. ...
4 years, 2 months ago (2015-09-16 07:38:24 UTC) #11
saroyanm
Patch uploaded. https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#newcode242 skin/firstRun.css:242: html[dir="rtl"] #general > div:not(:first-child) On 2015/09/16 07:38:24, ...
4 years, 2 months ago (2015-09-16 09:43:59 UTC) #12
Thomas Greiner
https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#newcode184 skin/firstRun.css:184: text-align: center; You still need to align the text ...
4 years, 2 months ago (2015-09-16 10:09:20 UTC) #13
Sebastian Noack
On 2015/09/16 10:09:20, Thomas Greiner wrote: > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css > File skin/firstRun.css (right): > > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#newcode184 ...
4 years, 2 months ago (2015-09-16 10:16:33 UTC) #14
saroyanm
https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#newcode184 skin/firstRun.css:184: text-align: center; On 2015/09/16 10:09:20, Thomas Greiner wrote: > ...
4 years, 2 months ago (2015-09-16 11:08:43 UTC) #15
saroyanm
On 2015/09/16 10:16:33, Sebastian Noack wrote: > On 2015/09/16 10:09:20, Thomas Greiner wrote: > > ...
4 years, 2 months ago (2015-09-16 11:11:03 UTC) #16
Sebastian Noack
On 2015/09/16 11:11:03, saroyanm wrote: > -webkit-border-start-width: 1px; > -moz-border-start-width: 1px; > > What about ...
4 years, 2 months ago (2015-09-16 11:14:37 UTC) #17
saroyanm
On 2015/09/16 11:14:37, Sebastian Noack wrote: > On 2015/09/16 11:11:03, saroyanm wrote: > > -webkit-border-start-width: ...
4 years, 2 months ago (2015-09-16 11:22:14 UTC) #18
Sebastian Noack
On 2015/09/16 11:22:14, saroyanm wrote: > On 2015/09/16 11:14:37, Sebastian Noack wrote: > > On ...
4 years, 2 months ago (2015-09-16 11:29:49 UTC) #19
saroyanm
On 2015/09/16 11:29:49, Sebastian Noack wrote: > On 2015/09/16 11:22:14, saroyanm wrote: > > On ...
4 years, 2 months ago (2015-09-16 11:37:37 UTC) #20
Sebastian Noack
On 2015/09/16 11:37:37, saroyanm wrote: > On 2015/09/16 11:29:49, Sebastian Noack wrote: > > Good ...
4 years, 2 months ago (2015-09-16 11:41:03 UTC) #21
saroyanm
On 2015/09/16 11:41:03, Sebastian Noack wrote: > On 2015/09/16 11:37:37, saroyanm wrote: > > On ...
4 years, 2 months ago (2015-09-16 11:46:26 UTC) #22
Sebastian Noack
On 2015/09/16 11:46:26, saroyanm wrote: > I'm not sure if it's reliable to use property ...
4 years, 2 months ago (2015-09-16 12:04:00 UTC) #23
Thomas Greiner
On 2015/09/16 12:04:00, Sebastian Noack wrote: > On 2015/09/16 11:46:26, saroyanm wrote: > > I'm ...
4 years, 2 months ago (2015-09-16 12:28:12 UTC) #24
Sebastian Noack
https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#newcode202 skin/firstRun.css:202: text-align: left; On 2015/09/16 12:28:12, Thomas Greiner wrote: > ...
4 years, 2 months ago (2015-09-16 12:32:33 UTC) #25
saroyanm
New patch uploaded. https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#newcode202 skin/firstRun.css:202: text-align: left; On 2015/09/16 12:32:33, Sebastian ...
4 years, 2 months ago (2015-09-16 12:47:21 UTC) #26
Sebastian Noack
LGTM
4 years, 2 months ago (2015-09-16 12:50:13 UTC) #27
Thomas Greiner
One more issue I found while testing. https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css#newcode205 skin/firstRun.css:205: display: inline-block; ...
4 years, 2 months ago (2015-09-16 14:09:08 UTC) #28
saroyanm
https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css#newcode205 skin/firstRun.css:205: display: inline-block; On 2015/09/16 14:09:08, Thomas Greiner wrote: > ...
4 years, 2 months ago (2015-09-16 14:11:59 UTC) #29
Thomas Greiner
4 years, 2 months ago (2015-09-16 14:44:05 UTC) #30
LGTM
Sign in to reply to this message.

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