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

Issue 29328672: Issue 2668 - Use new subscription types in options page (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by Thomas Greiner
Modified:
3 years, 8 months ago
Reviewers:
saroyanm
CC:
Felix Dahlke
Visibility:
Public.

Description

Note that while the feature strings are now in a common translation file, I decided to not use those for the first-run page yet to avoid complications with existing translations. I'll create a separate ticket about that which should be tackled as soon as #2834 is resolved.

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -55 lines) Patch
M ext/common.js View 2 chunks +4 lines, -2 lines 0 comments Download
A locale/en-US/common.json View 1 chunk +18 lines, -0 lines 0 comments Download
M options.js View 3 chunks +7 lines, -9 lines 4 comments Download
M subscriptions.xml View 1 chunk +55 lines, -44 lines 2 comments Download

Messages

Total messages: 4
Thomas Greiner
https://codereview.adblockplus.org/29328672/diff/29328673/options.js File options.js (right): https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321 options.js:321: var type = recommendation.type.replace(/\W/g, "_"); I needed to replace ...
3 years, 9 months ago (2015-09-25 15:43:22 UTC) #1
saroyanm
Just a few comments. https://codereview.adblockplus.org/29328672/diff/29328673/options.js File options.js (right): https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321 options.js:321: var type = recommendation.type.replace(/\W/g, "_"); ...
3 years, 8 months ago (2015-11-10 16:47:25 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29328672/diff/29328673/options.js File options.js (right): https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321 options.js:321: var type = recommendation.type.replace(/\W/g, "_"); On 2015/11/10 16:47:25, saroyanm ...
3 years, 8 months ago (2015-11-10 19:54:09 UTC) #3
saroyanm
3 years, 8 months ago (2015-11-11 10:47:57 UTC) #4
LGTM

https://codereview.adblockplus.org/29328672/diff/29328673/options.js
File options.js (right):

https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321
options.js:321: var type = recommendation.type.replace(/\W/g, "_");
On 2015/11/10 19:54:09, Thomas Greiner wrote:
> On 2015/11/10 16:47:25, saroyanm wrote:
> > On 2015/09/25 15:43:22, Thomas Greiner wrote:
> > > I needed to replace "-" with "_" for "common_feature_anti_adblock_title"
so
> to
> > > make it consistent with buildtools I also applied the same method in the
> block
> > > above:
https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l233
> > 
> > Nit: What is the reason to assign the result to variable if you are not
> reusing
> > it ?
> 
> Just a personal preference to improve readability since the variable name
> provides context to what this value is supposed to represent. I can replace
> those two lines with the following if you prefer that:
> 
> subscription.title = ext.i18n.getMessage("common_feature_"
>   + recommendation.type.replace(/\W/g, "_") + "_title");

I see, well while nothing related in coding styles and I can see similar
implementation in the file feel free to keep it as it is right now.
Sign in to reply to this message.

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