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

Issue 29502647: Issue 5482 - Sidebar and about ABP dialog (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by saroyanm
Modified:
2 years, 7 months ago
Reviewers:
juliandoucette
CC:
Thomas Greiner, ire
Visibility:
Public.

Patch Set 1 : #

Total comments: 56

Patch Set 2 : #

Total comments: 79

Patch Set 3 : Addressed comments #

Total comments: 4

Patch Set 4 : Rebased #

Total comments: 1

Patch Set 5 : Fixed tabulation #

Total comments: 29

Patch Set 6 : #

Patch Set 7 : Rebased #

Patch Set 8 : Styleguide changes #

Total comments: 2

Patch Set 9 : Fixed nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+451 lines, -502 lines) Patch
M locale/en-US/new-options.json View 1 2 3 4 5 6 7 3 chunks +26 lines, -16 lines 0 comments Download
M new-options.html View 1 2 3 4 5 6 2 chunks +262 lines, -264 lines 0 comments Download
M new-options.js View 1 2 3 4 5 6 11 chunks +13 lines, -51 lines 0 comments Download
A skin/abp-logo.svg View 1 chunk +1 line, -0 lines 0 comments Download
R skin/fonts/SourceSansPro-Semibold.woff View 1 2 3 4 5 6 7 Binary file 0 comments Download
A skin/fonts/SourceSansPro-bold.woff View 1 2 3 4 5 6 7 Binary file 0 comments Download
M skin/new-options.css View 1 2 3 4 5 6 7 8 10 chunks +149 lines, -171 lines 0 comments Download
M skin/options-sprite.png View Binary file 0 comments Download

Messages

Total messages: 24
saroyanm
This is ready for the review, references to styleguide and implementation specific links to the ...
2 years, 7 months ago (2017-08-10 12:04:49 UTC) #1
juliandoucette
Here are my first impressions. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#newcode38 new-options.html:38: <p class="i18n_options_page_title_2"></p> NIT: ~"Settings" ...
2 years, 7 months ago (2017-08-16 23:42:33 UTC) #2
saroyanm
https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#newcode38 new-options.html:38: <p class="i18n_options_page_title_2"></p> On 2017/08/16 23:42:31, juliandoucette wrote: > NIT: ...
2 years, 7 months ago (2017-08-18 10:23:14 UTC) #3
saroyanm
https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#newcode67 new-options.html:67: <a id="contribute" class="i18n_options_footer_contribute button secondary" On 2017/08/16 23:42:31, juliandoucette ...
2 years, 7 months ago (2017-08-18 10:49:29 UTC) #4
juliandoucette
Thanks Manvel, I think that I've responded to anything that you may have wanted my ...
2 years, 7 months ago (2017-08-18 11:17:04 UTC) #5
saroyanm
Thanks Julian for reviewing, the new patch is ready for review as well, please still ...
2 years, 7 months ago (2017-08-18 12:44:29 UTC) #6
juliandoucette
(Feedback before I review the new Patchset.) https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#newcode38 new-options.html:38: <p class="i18n_options_page_title_2"></p> ...
2 years, 7 months ago (2017-08-21 13:06:39 UTC) #7
juliandoucette
Finished reviewing latest Patchset. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json#newcode7 locale/en-US/new-options.json:7: "description": "Page title in navigation ...
2 years, 7 months ago (2017-08-21 14:10:35 UTC) #8
saroyanm
Thanks for reviwing Julian I answered your comments, I'm looking for your reply to them ...
2 years, 7 months ago (2017-08-21 15:20:23 UTC) #9
juliandoucette
Thanks Manvel, here are my replies. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json#newcode7 locale/en-US/new-options.json:7: "description": "Page title ...
2 years, 7 months ago (2017-08-21 16:07:26 UTC) #10
saroyanm
https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json#newcode7 locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/21 16:07:23, ...
2 years, 7 months ago (2017-08-21 16:46:14 UTC) #11
saroyanm
There are couple of things that are missing for final discussion, the discussions that are ...
2 years, 7 months ago (2017-08-21 17:02:55 UTC) #12
juliandoucette
Following up. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json#newcode7 locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On ...
2 years, 7 months ago (2017-08-22 10:10:45 UTC) #13
saroyanm
This is ready for review. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-options.json#newcode7 locale/en-US/new-options.json:7: "description": "Page title in ...
2 years, 7 months ago (2017-08-23 13:35:47 UTC) #14
juliandoucette
https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#newcode53 new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/23 13:35:44, saroyanm wrote: > This ...
2 years, 7 months ago (2017-08-23 18:11:12 UTC) #15
saroyanm
Thanks for the notes Julian, I think I have enough information to prepare new patch. ...
2 years, 7 months ago (2017-08-23 20:57:55 UTC) #16
juliandoucette
Following up. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#newcode45 new-options.html:45: role="tablist" data-action="switch-tab" On 2017/08/23 20:57:53, saroyanm wrote: ...
2 years, 7 months ago (2017-08-23 22:00:48 UTC) #17
saroyanm
I think this is ready for the review. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#newcode36 new-options.html:36: <img ...
2 years, 7 months ago (2017-08-24 17:39:45 UTC) #18
saroyanm
Rebased.
2 years, 7 months ago (2017-08-25 16:57:19 UTC) #19
saroyanm
This is ready for the review, but I wish to finish this review soon if ...
2 years, 7 months ago (2017-08-27 16:13:11 UTC) #20
juliandoucette
On 2017/08/27 16:13:11, saroyanm wrote: > * @Julian considering the fact that you have got ...
2 years, 7 months ago (2017-08-27 22:24:04 UTC) #21
juliandoucette
LGTM + NITs Another Patchset or Review will be necessary to address the REM font-size ...
2 years, 7 months ago (2017-08-28 11:16:42 UTC) #22
saroyanm
https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.css#newcode60 skin/new-options.css:60: font-size: 1rem; On 2017/08/28 11:16:41, juliandoucette wrote: > On ...
2 years, 7 months ago (2017-08-28 11:39:44 UTC) #23
juliandoucette
2 years, 7 months ago (2017-08-28 12:03:05 UTC) #24
LGTM
Sign in to reply to this message.

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