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

Issue 29442555: Issue 4934 - Add 'Requirements for Adblock Plus recommended filter lists' page to adblockplus.org

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by ire
Modified:
3 weeks, 2 days ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

Issue 4934 - Add 'Requirements for Adblock Plus recommended filter lists' page to adblockplus.org

Patch Set 1 #

Total comments: 4

Patch Set 2 : Added missing <fix>, Remove space around string brackets, Replace angled quotes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -0 lines) Patch
M pages/contribute.html View 1 1 chunk +4 lines, -0 lines 0 comments Download
A pages/filter-lists-requirements.html View 1 1 chunk +49 lines, -0 lines 0 comments Download

Messages

Total messages: 2
ire
1 month ago (2017-05-19 05:09:49 UTC) #1
juliandoucette
3 weeks, 2 days ago (2017-05-30 12:55:03 UTC) #2
Thanks Ire! :)

There are a few minor issues.

Note: When I find a NIT (e.g. missing <fix>, unnecessary UTF-8) I leave a
comment on one line (not everywhere it occurs). So please check the rest of the
changeset for more occurrences.

https://codereview.adblockplus.org/29442555/diff/29442556/pages/contribute.html
File pages/contribute.html (right):

https://codereview.adblockplus.org/29442555/diff/29442556/pages/contribute.ht...
pages/contribute.html:213: {{ filter-lists-requirements-paragraph[Filter Lists
Requirements Paragraph] Adblock Plus is only as good as its filters. Share your
filters, suggest new ones or help communities of volunteer contributors to draft
and update filter lists. These filter lists need to be constantly maintained to
block menaces like new tracking systems, malware threats and intrusive ads.
Click <a href="https://adblockplus.org/filter-lists-requirements">here</a> for a
list of requirements. }}
NIT: Please remove "https://adblockplus.org/" from this url. cms HTMLConverter
automatically resolves and localizes anchors by page name and language.

https://codereview.adblockplus.org/29442555/diff/29442556/pages/filter-lists-...
File pages/filter-lists-requirements.html (right):

https://codereview.adblockplus.org/29442555/diff/29442556/pages/filter-lists-...
pages/filter-lists-requirements.html:4: <p>{{
filter-lists-requirements-intro[Filter Lists Requirements Intro] The list:
}}</p>
NIT: Please remove spaces before string IDs

( PS: I had no idea that we could put a space there :D - We should be consistent
with other pages anyhow...)

https://codereview.adblockplus.org/29442555/diff/29442556/pages/filter-lists-...
pages/filter-lists-requirements.html:15: {{
filter-lists-requirements-item2-example[Filter Lists Requirements List Item]
Example: we will not support a second German filter list, or a list that has
filters already contained in EasyList (i.e. do not create another “EasyList
Germany” if it does not include additional or better filters) }}
NIT: Missing <fix> around EasyList

https://codereview.adblockplus.org/29442555/diff/29442556/pages/filter-lists-...
pages/filter-lists-requirements.html:24: {{
filter-lists-requirements-item3-example[Filter Lists Requirements List Item]
Example: should have no redundancy, no extremely generic filters, no <a
href="https://adblockplus.org/forum/viewtopic.php?t=6118">“slow” filters</a> }}
NIT: Please replace angled commas and quotes
Sign in to reply to this message.

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