Issue 29340844: Issue 3802 - Create page to inform users about adware on web.adblockplus.org

juliandoucette

April 26, 2016, 5:05 p.m. (2016-04-26 17:05:47 UTC) #1

saroyanm

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html File pages/adware.html (right): https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#newcode1 pages/adware.html:1: title=About adware We usually using Markdown where it's possible, ...

May 3, 2016, 10:06 a.m. (2016-05-03 10:06:33 UTC) #2

juliandoucette

Patches soon to come. https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html File pages/adware.html (right): https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#newcode1 pages/adware.html:1: title=About adware On 2016/05/03 10:06:30, ...

May 11, 2016, 12:45 a.m. (2016-05-11 00:45:41 UTC) #3

Patches soon to come.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html
File pages/adware.html (right):

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:1: title=About adware
On 2016/05/03 10:06:30, saroyanm wrote:
> We usually using Markdown where it's possible, what is the reason of using
.html
> instead ?

If this document was .md then we could replace:

- p
- ul (Without icons)
- strong

with markdown.

Considering the type and amount of code that remains, there is very little
benefit in using markdown.

With that said, it makes no difference to me if I change it to markdown and
remove the tags where I can. Do you want me to do that?

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:5: <meta name="description" content="What is adware and how to
get rid of it">
On 2016/05/03 10:06:30, saroyanm wrote:
> we can also use open graph version for this tag.

For what reason?

This is the standard meta tag.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:6: <meta name="author" content="Eyeo GmbH / Adblock Plus">
On 2016/05/03 10:06:31, saroyanm wrote:
> I think we can use this meta tag in the Template itself, not to add it to each
> post individually.

Nope. It's not in the default template. Would you like me to add it to the
default template?

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:8: <meta name="twitter:card" content="summary">
On 2016/05/03 10:06:32, saroyanm wrote:
> Seems like the twitter cards should also use @ sign for the twitter handle
> "@AdblockPlus":
> https://dev.twitter.com/cards/getting-started#opengraph

Good catch.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:12: .text-danger
On 2016/05/03 10:06:32, saroyanm wrote:
> You can have a wrapper for this reason, ex.: "warning" and in that case you
> won't need to assign this color to heading separately and avoid duplication,
in
> general I'd say that the warning  header should go inside of the box (in the
> current design), but with current design it's outside, but it still make sense
> to wrap, because it's still part of the warning I assume.

I agree.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:14: color: #E11A2C
On 2016/05/03 10:06:31, saroyanm wrote:
> Detail: please use lower case for the css values ->
>
https://google.github.io/styleguide/htmlcssguide.xml?showone=Capitalization#C...

I don't think we should follow that rule.

Done anyway.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:37: padding: 7px 0;
On 2016/05/03 10:06:30, saroyanm wrote:
> Detail: Please reorder the properties according to the style-guide: 
>
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/...
> 
> Same applies to other selector properties.
> Also please keep current rule "CSS number values should specify units where
> possible"

I don't think we should follow that rule.

Done anyway.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:96: .list-custom .li-yes:before,
On 2016/05/03 10:06:32, saroyanm wrote:
> Please do not use element related class names. 

If not li-yes, li-no is check-item, cross-item ok? Can you suggest something
else?

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:104: width: 40px;
On 2016/05/03 10:06:31, saroyanm wrote:
> Width of the icon should be same size as the icon itself. So you probably will
> need to have 2 separate icon selectors or you can try to ask Christiane to
> create icon of same size

- I do have 2 separate icon selectors .li-yes, .li-no
- The icons should be SVG or higher resolution (the resolution provided is
ideal) for high DPI screens

The icon sizes are scaled proportionally based on their height.
- check to 14px like the style guide
- cross to 18px like the style guide

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:112: background-image: url("/img/check.png");
On 2016/05/03 10:06:30, saroyanm wrote:
> The image size according to style-guide is 20x14px, please re-size the icon to
> make it the correct size and also you can proceed image through pngout:
> http://www.advsys.net/ken/util/pngout.htm 
> To make the file size smaller

The image should be SVG or higher resolution (the resolution provided is ideal)
for high DPI screens.

I will optimize the image. Let me know if you think it is still too heavy (then
we should probably use SVG).

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:122: #content .hr-top
On 2016/05/03 10:06:32, saroyanm wrote:
> You can apply this style to the section element itself:
> select > select, or have a class on parent, not to assign style to each
element.

You mean "section" not "select" :D ... That confused me for a minute.

I was trying to make my css more portable.

Done.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:127: #content h3
On 2016/05/03 10:06:30, saroyanm wrote:
> In general would be great to have as less page specific selectors as possible,
> because in the end we should have general styling for all the pages.
> Also I think this is more looks to be list element.

I agree / acknowledge. 

In this case, I think H3 is better for the content outline + SEO, while still
being consistant with the rest of the site (not using H1 everywhere like HTML5
would have you).

I will change this if you insist.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:136: <h1>{{adware-page-header I’m using Adblock Plus but I
still see suspicious ads}}</h1>
On 2016/05/03 10:06:31, saroyanm wrote:
> Detail: No need to use adware in the string name to make it unique, because
> stringIDs are assigned per page.

Thanks!

I didn't know this at the time.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:139: <h2 class="text-danger short">{{adware-alert-heading What
happened?}}</h2>
On 2016/05/03 10:06:31, saroyanm wrote:
> No style is assigned to class "short"

Good catch! That was a leftover :-) .

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:159: <table class="table-unstyled">
On 2016/05/03 10:06:32, saroyanm wrote:
> Please do not use Table for layouting, by changing this you can make this
block
> responsive.

I believe this is tabular data?

(It also looks fine on small phones without stacking)

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:166: <td><a href="{{malwarebytes-win-affiliate-link
buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}">{{malwarebytes-brand
Malwarebytes}}</a></td>
On 2016/05/03 10:06:31, saroyanm wrote:
> 1. I think this is wrong, I think we shouldn't translate the links and rely on
> third party to keep the translated pages, this should be handled on their side
> according to the user setup in his browser (same applies to other similar
> links).
> 2. This link is relative please add "http" in the begining.

I disagree.

We do not have affiliate links for all languages. Therefore, we have to
whitelist the link and default to English somewhere. And I think our translation
system, which already does this by default, is better than hard coding the
whitelist in python inside one of our templates.

Will you reconsider?

juliandoucette

On 2016/05/11 00:45:41, juliandoucette wrote: > Patches soon to come. > > https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html > File ...

May 11, 2016, 1:10 a.m. (2016-05-11 01:10:12 UTC) #4

On 2016/05/11 00:45:41, juliandoucette wrote:
> Patches soon to come.
> 
> https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html
> File pages/adware.html (right):
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:1: title=About adware
> On 2016/05/03 10:06:30, saroyanm wrote:
> > We usually using Markdown where it's possible, what is the reason of using
> .html
> > instead ?
> 
> If this document was .md then we could replace:
> 
> - p
> - ul (Without icons)
> - strong
> 
> with markdown.
> 
> Considering the type and amount of code that remains, there is very little
> benefit in using markdown.
> 
> With that said, it makes no difference to me if I change it to markdown and
> remove the tags where I can. Do you want me to do that?
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:5: <meta name="description" content="What is adware and how
to
> get rid of it">
> On 2016/05/03 10:06:30, saroyanm wrote:
> > we can also use open graph version for this tag.
> 
> For what reason?
> 
> This is the standard meta tag.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:6: <meta name="author" content="Eyeo GmbH / Adblock Plus">
> On 2016/05/03 10:06:31, saroyanm wrote:
> > I think we can use this meta tag in the Template itself, not to add it to
each
> > post individually.
> 
> Nope. It's not in the default template. Would you like me to add it to the
> default template?
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:8: <meta name="twitter:card" content="summary">
> On 2016/05/03 10:06:32, saroyanm wrote:
> > Seems like the twitter cards should also use @ sign for the twitter handle
> > "@AdblockPlus":
> > https://dev.twitter.com/cards/getting-started#opengraph
> 
> Good catch.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:12: .text-danger
> On 2016/05/03 10:06:32, saroyanm wrote:
> > You can have a wrapper for this reason, ex.: "warning" and in that case you
> > won't need to assign this color to heading separately and avoid duplication,
> in
> > general I'd say that the warning  header should go inside of the box (in the
> > current design), but with current design it's outside, but it still make
sense
> > to wrap, because it's still part of the warning I assume.
> 
> I agree.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:14: color: #E11A2C
> On 2016/05/03 10:06:31, saroyanm wrote:
> > Detail: please use lower case for the css values ->
> >
>
https://google.github.io/styleguide/htmlcssguide.xml?showone=Capitalization#C...
> 
> I don't think we should follow that rule.
> 
> Done anyway.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:37: padding: 7px 0;
> On 2016/05/03 10:06:30, saroyanm wrote:
> > Detail: Please reorder the properties according to the style-guide: 
> >
>
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/...
> > 
> > Same applies to other selector properties.
> > Also please keep current rule "CSS number values should specify units where
> > possible"
> 
> I don't think we should follow that rule.
> 
> Done anyway.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:96: .list-custom .li-yes:before,
> On 2016/05/03 10:06:32, saroyanm wrote:
> > Please do not use element related class names. 
> 
> If not li-yes, li-no is check-item, cross-item ok? Can you suggest something
> else?
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:104: width: 40px;
> On 2016/05/03 10:06:31, saroyanm wrote:
> > Width of the icon should be same size as the icon itself. So you probably
will
> > need to have 2 separate icon selectors or you can try to ask Christiane to
> > create icon of same size
> 
> - I do have 2 separate icon selectors .li-yes, .li-no
> - The icons should be SVG or higher resolution (the resolution provided is
> ideal) for high DPI screens
> 
> The icon sizes are scaled proportionally based on their height.
> - check to 14px like the style guide
> - cross to 18px like the style guide
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:112: background-image: url("/img/check.png");
> On 2016/05/03 10:06:30, saroyanm wrote:
> > The image size according to style-guide is 20x14px, please re-size the icon
to
> > make it the correct size and also you can proceed image through pngout:
> > http://www.advsys.net/ken/util/pngout.htm 
> > To make the file size smaller
> 
> The image should be SVG or higher resolution (the resolution provided is
ideal)
> for high DPI screens.
> 
> I will optimize the image. Let me know if you think it is still too heavy
(then
> we should probably use SVG).
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:122: #content .hr-top
> On 2016/05/03 10:06:32, saroyanm wrote:
> > You can apply this style to the section element itself:
> > select > select, or have a class on parent, not to assign style to each
> element.
> 
> You mean "section" not "select" :D ... That confused me for a minute.
> 
> I was trying to make my css more portable.
> 
> Done.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:127: #content h3
> On 2016/05/03 10:06:30, saroyanm wrote:
> > In general would be great to have as less page specific selectors as
possible,
> > because in the end we should have general styling for all the pages.
> > Also I think this is more looks to be list element.
> 
> I agree / acknowledge. 
> 
> In this case, I think H3 is better for the content outline + SEO, while still
> being consistant with the rest of the site (not using H1 everywhere like HTML5
> would have you).
> 
> I will change this if you insist.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:136: <h1>{{adware-page-header I’m using Adblock Plus but I
> still see suspicious ads}}</h1>
> On 2016/05/03 10:06:31, saroyanm wrote:
> > Detail: No need to use adware in the string name to make it unique, because
> > stringIDs are assigned per page.
> 
> Thanks!
> 
> I didn't know this at the time.
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:139: <h2 class="text-danger short">{{adware-alert-heading
What
> happened?}}</h2>
> On 2016/05/03 10:06:31, saroyanm wrote:
> > No style is assigned to class "short"
> 
> Good catch! That was a leftover :-) .
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:159: <table class="table-unstyled">
> On 2016/05/03 10:06:32, saroyanm wrote:
> > Please do not use Table for layouting, by changing this you can make this
> block
> > responsive.
> 
> I believe this is tabular data?
> 
> (It also looks fine on small phones without stacking)
> 
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> pages/adware.html:166: <td><a href="{{malwarebytes-win-affiliate-link
>
buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}">{{malwarebytes-brand
> Malwarebytes}}</a></td>
> On 2016/05/03 10:06:31, saroyanm wrote:
> > 1. I think this is wrong, I think we shouldn't translate the links and rely
on
> > third party to keep the translated pages, this should be handled on their
side
> > according to the user setup in his browser (same applies to other similar
> > links).
> > 2. This link is relative please add "http" in the begining.
> 
> I disagree.
> 
> We do not have affiliate links for all languages. Therefore, we have to
> whitelist the link and default to English somewhere. And I think our
translation
> system, which already does this by default, is better than hard coding the
> whitelist in python inside one of our templates.
> 
> Will you reconsider?

Also, about using markdown instead of html:

- We cannot currently use markdown without removing indentation
-- Indentation past a point creates a `pre`
- We cannot currently use markdown inside of HTML elements (EG in a table,
custom list element, custom text element, or subsection)
-- As a result, we would not currently be able to section our document properly
in HTML5

juliandoucette

Also, about using markdown instead of html: - We cannot currently use markdown without removing ...

May 11, 2016, 1:12 a.m. (2016-05-11 01:12:09 UTC) #5

saroyanm

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html File pages/adware.html (right): https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#newcode1 pages/adware.html:1: title=About adware On 2016/05/11 00:45:39, juliandoucette wrote: > On ...

May 17, 2016, 10:19 a.m. (2016-05-17 10:19:44 UTC) #6

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html
File pages/adware.html (right):

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:1: title=About adware
On 2016/05/11 00:45:39, juliandoucette wrote:
> On 2016/05/03 10:06:30, saroyanm wrote:
> > We usually using Markdown where it's possible, what is the reason of using
> .html
> > instead ?
> 
> If this document was .md then we could replace:
> 
> - p
> - ul (Without icons)
> - strong
> 
> with markdown.
> 
> Considering the type and amount of code that remains, there is very little
> benefit in using markdown.
> 
> With that said, it makes no difference to me if I change it to markdown and
> remove the tags where I can. Do you want me to do that?

There is a benefit, because I assume soon or later we will need to move most of
our pages to Markdown and ease developers job in most of the future changes,
that why we need to keep the pages consistent.
So yes It's better to move this to markdown, also consider the fact that we have
Markdown extra enabled:
https://pythonhosted.org/Markdown/extensions/extra.html as mentioned here ->
https://hg.adblockplus.org/cms/file/tip/README.md#l215
Which allows you to use markdown inside of block level element and also specify
classes and IDs on the elements using Attribute list extension ->
https://pythonhosted.org/Markdown/extensions/attr_list.html

Side note:
Also I'm not sure if it's good idea to introduce a lot of style changes that we
did for this project because in the end it should a general page and we will
need to adapt all of our pages to have same styling, I hope that all this
inconsistencies will be fixed with the introduction of the Help center, but it's
not encouraged to have a lot of style changes because this can lead to
complications with designing separately individual pages.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:5: <meta name="description" content="What is adware and how to
get rid of it">
On 2016/05/11 00:45:38, juliandoucette wrote:
> On 2016/05/03 10:06:30, saroyanm wrote:
> > we can also use open graph version for this tag.
> 
> For what reason?
> 
> This is the standard meta tag.

For Social media:
https://developers.facebook.com/docs/sharing/best-practices#tags

Also Twitter using special Open graphs:
https://dev.twitter.com/cards/markup#twitter-description

But if usual description works fine over the social media platforms we can leave
it, otherwise according to the tags below you used feels like you are trying to
optimize for Twitter, so in that case make sense to use also the meta
information suggested by that or more platforms.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:6: <meta name="author" content="Eyeo GmbH / Adblock Plus">
On 2016/05/11 00:45:39, juliandoucette wrote:
> On 2016/05/03 10:06:31, saroyanm wrote:
> > I think we can use this meta tag in the Template itself, not to add it to
each
> > post individually.
> 
> Nope. It's not in the default template. Would you like me to add it to the
> default template?

I assume this is relevant to all other documentation on our website, so maybe we
can add it in general to the template, because otherwise if we will start to use
that meta tag for each of other page this can end up in to misspell issues and
we will need to update the tag separately, so that why I assume it make sense to
use general tag that is relevant to all pages in the template.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:14: color: #E11A2C
On 2016/05/11 00:45:39, juliandoucette wrote:
> On 2016/05/03 10:06:31, saroyanm wrote:
> > Detail: please use lower case for the css values ->
> >
>
https://google.github.io/styleguide/htmlcssguide.xml?showone=Capitalization#C...
> 
> I don't think we should follow that rule.
Why ? We are using this general rules in our Coding style:
https://adblockplus.org/en/coding-style#html-css 
"Follow the Google HTML/CSS Style Guide."

> Done anyway.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:37: padding: 7px 0;
On 2016/05/11 00:45:40, juliandoucette wrote:
> On 2016/05/03 10:06:30, saroyanm wrote:
> > Detail: Please reorder the properties according to the style-guide: 
> >
>
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/...
> > 
> > Same applies to other selector properties.
> > Also please keep current rule "CSS number values should specify units where
> > possible"
> 
> I don't think we should follow that rule.
Same as above, we are using that rule as you can see from our coding style
document:
"CSS rule declaration order should follow the WordPress CSS Coding Standards" 
> Done anyway.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:96: .list-custom .li-yes:before,
On 2016/05/11 00:45:40, juliandoucette wrote:
> On 2016/05/03 10:06:32, saroyanm wrote:
> > Please do not use element related class names. 
> 
> If not li-yes, li-no is check-item, cross-item ok? Can you suggest something
> else?

The idea behind is that you can assign the rule to other elements as well, or in
case you will change the element you will not need to adapt the css name, so
IMHO we shouldn't use element specific names:
Ex.: icon-check ?

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:104: width: 40px;
On 2016/05/11 00:45:39, juliandoucette wrote:
> On 2016/05/03 10:06:31, saroyanm wrote:
> > Width of the icon should be same size as the icon itself. So you probably
will
> > need to have 2 separate icon selectors or you can try to ask Christiane to
> > create icon of same size
> 
> - I do have 2 separate icon selectors .li-yes, .li-no
> - The icons should be SVG or higher resolution (the resolution provided is
> ideal) for high DPI screens
SVG is not supported in the browser that we still support, so no we can't use
SVG yet.
Unfortunately I need to argue that we can not support higher resolutions yet,
but maybe in couple of years, because currently people will be forced to
download bigger image to show the same result, I think we need to wait for a
longer time until better solution is being supported by the browsers we are
using, so it will take a year or more I think, otherwise we will need to
implement our own solution, but that will require using Javascript to let the
browser download the icon according to whether the device support high DPI
screen, so it's not ideal. My suggestion is sticking with the actual size and
not make icons bigger. I'm looking forward for the "picture" element to fix all
that issues, but not now unfortunately.

> The icon sizes are scaled proportionally based on their height.
> - check to 14px like the style guide
> - cross to 18px like the style guide
I know, but it could have ease the styling, anyway yes, if the icons already
provided on different sizes easiest will be to create separate style for each of
the icons.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:112: background-image: url("/img/check.png");
On 2016/05/11 00:45:40, juliandoucette wrote:
> On 2016/05/03 10:06:30, saroyanm wrote:
> > The image size according to style-guide is 20x14px, please re-size the icon
to
> > make it the correct size and also you can proceed image through pngout:
> > http://www.advsys.net/ken/util/pngout.htm 
> > To make the file size smaller
> 
> The image should be SVG or higher resolution (the resolution provided is
ideal)
> for high DPI screens.
> 
> I will optimize the image. Let me know if you think it is still too heavy
(then
> we should probably use SVG).

Same as the comment above,

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:159: <table class="table-unstyled">
On 2016/05/11 00:45:40, juliandoucette wrote:
> On 2016/05/03 10:06:32, saroyanm wrote:
> > Please do not use Table for layouting, by changing this you can make this
> block
> > responsive.
> 
> I believe this is tabular data?
> 
> (It also looks fine on small phones without stacking)

Unfortunately Tables are not responsive at all and so it's better to find other
solution, also I assume we are planing to add some more columns and in that case
this will have issues also on phones. So I'll suggest to change this also it can
be consistent with other list items on the page.

https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
pages/adware.html:166: <td><a href="{{malwarebytes-win-affiliate-link
buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}">{{malwarebytes-brand
Malwarebytes}}</a></td>
On 2016/05/11 00:45:39, juliandoucette wrote:
> On 2016/05/03 10:06:31, saroyanm wrote:
> > 1. I think this is wrong, I think we shouldn't translate the links and rely
on
> > third party to keep the translated pages, this should be handled on their
side
> > according to the user setup in his browser (same applies to other similar
> > links).
> > 2. This link is relative please add "http" in the begining.
> 
> I disagree.
> 
> We do not have affiliate links for all languages. Therefore, we have to
> whitelist the link and default to English somewhere. And I think our
translation
> system, which already does this by default, is better than hard coding the
> whitelist in python inside one of our templates.
> 
> Will you reconsider?

Sorry I think I couldn't describe my point well, what I meant is using:
http://buy.malwarebytes.com/get-trial/adblock/ instead of
http://buy.malwarebytes.com/get-trial/adblock/en
http://www.surfright.nl/hitmanpro instead of
http://www.surfright.nl/en/hitmanpro
Same you did for: 
https://toolslib.net/downloads/viewdownload/1-adwcleaner 

I do not know why we are forced to translate all the links.
Also I'm not sure if this will be supported in our CMS because currently we
translate links like:
"text<a>link text</a> text"

juliandoucette

# Patch set 4 - Moved relevant meta tags from page to default template - ...

May 23, 2016, 7:51 p.m. (2016-05-23 19:51:13 UTC) #7

saroyanm

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css#newcode418 static/css/main.css:418: This styles are for just a custom page we ...

May 31, 2016, 3:23 p.m. (2016-05-31 15:23:36 UTC) #8

juliandoucette

Another patch added. Thanks Manvel. I changed everything to your recommendation except for meta tags ...

May 31, 2016, 7:27 p.m. (2016-05-31 19:27:47 UTC) #9

Another patch added. Thanks Manvel.

I changed everything to your recommendation except for meta tags in the header.

Please see my previous comments.

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css...
static/css/main.css:418: 
On 2016/05/31 15:23:35, saroyanm wrote:
> This styles are for just a custom page we have, doesn't make sense to load
them
> for every page we have now. They do not belong here IMO:
> You can still use.
> <head>
>   <style type="text/css">
>   </style>
> </head>
> 
> In adware.md to apply custom styles for specific page.

I'm not sure:

- by your logic, what styles **do** belong in main.css?
  - I thought that these styles were mostly general/basic enough
- aren't we trying to avoid putting many styles inside pages because pages are
meant to be _content_ and not _code_?

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css...
static/css/main.css:419: .split-section {
On 2016/05/31 15:23:35, saroyanm wrote:
> detail: "Opening braces always go on their own line" according to our coding
> style, same applies to the styles below.

Done.

Sorry about that :/

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css...
static/css/main.css:428: margin: 0 35px 10px 0px;
On 2016/05/31 15:23:35, saroyanm wrote:
> detail: "CSS number values should specify units where possible"  according to
> our coding style, same applies to the styles below.

Done.

Sorry about that :/

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:24: <title>{{title|translate("title")}}</title>
On 2016/05/31 15:23:36, saroyanm wrote:
> What was the reason of moving the title here ? 

I grouped these tags based on *what kind of data they describe*.

In this grouping:

- meta and viewport describe the rendering of the document
- title, author, description, ... describe the content of the document

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:25: <meta name="author"
content="{{author|translate("author") if author else get_string("Eyeo GmbH /
Adblock Plus", "meta")}}" />
On 2016/05/31 15:23:35, saroyanm wrote:
> This is not how get_string is being used, this causes error:
> https://hg.adblockplus.org/cms/file/tip/README.md#l288 
> I assume your intention was to use translate filter:
> https://hg.adblockplus.org/cms/file/tip/README.md#l273

Thanks!

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:26: {% if description %}
On 2016/05/31 15:23:36, saroyanm wrote:
> I'm not sure if introduction of this conditions are good idea seems like you
> only using description variable in adware.md, but not "og_title", "og:type",
> "og:url", so I think it make sense to only introduce twitter cards here and
also
> without check for the "twitter_card", "twitter_site" and "twitter_creator"
> variables, we will add them when we will use them elsewhere, also probably
> doesn't make sense to devide "title" and "og:title", "description" and
> "og:description" because mostly I think they should have same content and
> doesn't make sense to translate them separately, anyway we can align regarding
> that in separate review, let's leave it for later.

- I made description conditional because some pages already define it inside of
their <head>
- I made title/og:title and description/og:description in case we want to target
social media differently than search enignes. EG: Providing a description with
less than 140 characters for twitter OR providing a more provocative description
for Facebook OR providing a a better targeted description for specific search
results. 

You give the word, and I'll remove them, just thought I would explain a little
better.

saroyanm

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css#newcode418 static/css/main.css:418: On 2016/05/31 19:27:46, juliandoucette wrote: > On 2016/05/31 15:23:35, ...

June 1, 2016, 2:38 p.m. (2016-06-01 14:38:02 UTC) #10

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css...
static/css/main.css:418: 
On 2016/05/31 19:27:46, juliandoucette wrote:
> On 2016/05/31 15:23:35, saroyanm wrote:
> > This styles are for just a custom page we have, doesn't make sense to load
> them
> > for every page we have now. They do not belong here IMO:
> > You can still use.
> > <head>
> >   <style type="text/css">
> >   </style>
> > </head>
> > 
> > In adware.md to apply custom styles for specific page.
> 
> I'm not sure:
> 
> - by your logic, what styles **do** belong in main.css?
>   - I thought that these styles were mostly general/basic enough
> - aren't we trying to avoid putting many styles inside pages because pages are
> meant to be _content_ and not _code_?

That's true and would be great not to have too many custom styles for specific
page, but while it's only this page I think we need to make it custom, but if we
will start to use that styles on other pages we can move them to main.css, or
create separate template for specific pages in case it's only general for them,
but while web.adblockplus.org is messy, we need try not to provide more mess, so
I think it's better to keep the style for this custom case I think.

So for future projects we will need to understand what kind of element specific
pages need to use and create specific template for them, in case of
web.adblockplus.org it's hard to find good balance and in the end after content
migration to help center we will need to think about the redesign and clearing
the mess :)

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:26: {% if description %}
On 2016/05/31 19:27:46, juliandoucette wrote:
> On 2016/05/31 15:23:36, saroyanm wrote:
> > I'm not sure if introduction of this conditions are good idea seems like you
> > only using description variable in adware.md, but not "og_title", "og:type",
> > "og:url", so I think it make sense to only introduce twitter cards here and
> also
> > without check for the "twitter_card", "twitter_site" and "twitter_creator"
> > variables, we will add them when we will use them elsewhere, also probably
> > doesn't make sense to devide "title" and "og:title", "description" and
> > "og:description" because mostly I think they should have same content and
> > doesn't make sense to translate them separately, anyway we can align
regarding
> > that in separate review, let's leave it for later.
> 
> - I made description conditional because some pages already define it inside
of
> their <head>
> - I made title/og:title and description/og:description in case we want to
target
> social media differently than search enignes. EG: Providing a description with
> less than 140 characters for twitter OR providing a more provocative
description
> for Facebook OR providing a a better targeted description for specific search
> results. 
> You give the word, and I'll remove them, just thought I would explain a little
> better.

What about updating the logic to something like:
{% if description %}
  <meta name="description" content="{{description|translate("description")}}" />
  {% if og_description %}
    <meta property="og:description"
content="{{og_description|translate("og_description")}}" />
  {% else %}
    <meta property="og:description" content="{{description}}" />
  {% endif %}
{% endif %}

I think same can work for the title, not sure if we are planing to provide
og:title separately too often.

We mostly used to have same description for general description and open graph,
so we need to try not to have duplicated translations, otherwise the translation
needs to be done for the same string separately

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.t...
templates/default.tmpl:35: {% if og_url %}
This is not being used anywhere, we can add it as soon we will use og:url, or
maybe this should be same as our canonical URL ? I'm not really sure what og:url
meta tag used for.

juliandoucette

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl#newcode35 templates/default.tmpl:35: {% if og_url %} On 2016/06/01 14:38:01, saroyanm wrote: ...

June 1, 2016, 3:56 p.m. (2016-06-01 15:56:39 UTC) #11

saroyanm

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl#newcode35 templates/default.tmpl:35: {% if og_url %} On 2016/06/01 15:56:38, juliandoucette wrote: ...

June 1, 2016, 3:59 p.m. (2016-06-01 15:59:29 UTC) #12

juliandoucette

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl#newcode35 templates/default.tmpl:35: {% if og_url %} On 2016/06/01 15:59:28, saroyanm wrote: ...

June 1, 2016, 4:09 p.m. (2016-06-01 16:09:50 UTC) #13

juliandoucette

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css#newcode418 static/css/main.css:418: On 2016/06/01 14:38:01, saroyanm wrote: > On 2016/05/31 19:27:46, ...

June 1, 2016, 4:23 p.m. (2016-06-01 16:23:20 UTC) #14

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29340844/diff/29342978/static/css/main.css...
static/css/main.css:418: 
On 2016/06/01 14:38:01, saroyanm wrote:
> On 2016/05/31 19:27:46, juliandoucette wrote:
> > On 2016/05/31 15:23:35, saroyanm wrote:
> > > This styles are for just a custom page we have, doesn't make sense to load
> > them
> > > for every page we have now. They do not belong here IMO:
> > > You can still use.
> > > <head>
> > >   <style type="text/css">
> > >   </style>
> > > </head>
> > > 
> > > In adware.md to apply custom styles for specific page.
> > 
> > I'm not sure:
> > 
> > - by your logic, what styles **do** belong in main.css?
> >   - I thought that these styles were mostly general/basic enough
> > - aren't we trying to avoid putting many styles inside pages because pages
are
> > meant to be _content_ and not _code_?
> 
> That's true and would be great not to have too many custom styles for specific
> page, but while it's only this page I think we need to make it custom, but if
we
> will start to use that styles on other pages we can move them to main.css, or
> create separate template for specific pages in case it's only general for
them,
> but while http://web.adblockplus.org is messy, we need try not to provide more
mess, so
> I think it's better to keep the style for this custom case I think.
> 
> So for future projects we will need to understand what kind of element
specific
> pages need to use and create specific template for them, in case of
> http://web.adblockplus.org it's hard to find good balance and in the end after
content
> migration to help center we will need to think about the redesign and clearing
> the mess :)

Done.

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:26: {% if description %}
On 2016/06/01 14:38:01, saroyanm wrote:
> On 2016/05/31 19:27:46, juliandoucette wrote:
> > On 2016/05/31 15:23:36, saroyanm wrote:
> > > I'm not sure if introduction of this conditions are good idea seems like
you
> > > only using description variable in adware.md, but not "og_title",
"og:type",
> > > "og:url", so I think it make sense to only introduce twitter cards here
and
> > also
> > > without check for the "twitter_card", "twitter_site" and "twitter_creator"
> > > variables, we will add them when we will use them elsewhere, also probably
> > > doesn't make sense to devide "title" and "og:title", "description" and
> > > "og:description" because mostly I think they should have same content and
> > > doesn't make sense to translate them separately, anyway we can align
> regarding
> > > that in separate review, let's leave it for later.
> > 
> > - I made description conditional because some pages already define it inside
> of
> > their <head>
> > - I made title/og:title and description/og:description in case we want to
> target
> > social media differently than search enignes. EG: Providing a description
with
> > less than 140 characters for twitter OR providing a more provocative
> description
> > for Facebook OR providing a a better targeted description for specific
search
> > results. 
> > You give the word, and I'll remove them, just thought I would explain a
little
> > better.
> 
> What about updating the logic to something like:
> {% if description %}
>   <meta name="description" content="{{description|translate("description")}}"
/>
>   {% if og_description %}
>     <meta property="og:description"
> content="{{og_description|translate("og_description")}}" />
>   {% else %}
>     <meta property="og:description" content="{{description}}" />
>   {% endif %}
> {% endif %}
> 
> I think same can work for the title, not sure if we are planing to provide
> og:title separately too often.
> 
> We mostly used to have same description for general description and open
graph,
> so we need to try not to have duplicated translations, otherwise the
translation
> needs to be done for the same string separately

I don't see the benefit in this change?
- it breaks the grouping html/open graph/twitter
- og:description defaults to html5 description by default

saroyanm

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl#newcode26 templates/default.tmpl:26: {% if description %} On 2016/06/01 16:23:19, juliandoucette wrote: ...

June 2, 2016, 3:05 p.m. (2016-06-02 15:05:12 UTC) #15

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:26: {% if description %}
On 2016/06/01 16:23:19, juliandoucette wrote:
> On 2016/06/01 14:38:01, saroyanm wrote:
> > On 2016/05/31 19:27:46, juliandoucette wrote:
> > > On 2016/05/31 15:23:36, saroyanm wrote:
> > > > I'm not sure if introduction of this conditions are good idea seems like
> you
> > > > only using description variable in adware.md, but not "og_title",
> "og:type",
> > > > "og:url", so I think it make sense to only introduce twitter cards here
> and
> > > also
> > > > without check for the "twitter_card", "twitter_site" and
"twitter_creator"
> > > > variables, we will add them when we will use them elsewhere, also
probably
> > > > doesn't make sense to devide "title" and "og:title", "description" and
> > > > "og:description" because mostly I think they should have same content
and
> > > > doesn't make sense to translate them separately, anyway we can align
> > regarding
> > > > that in separate review, let's leave it for later.
> > > 
> > > - I made description conditional because some pages already define it
inside
> > of
> > > their <head>
> > > - I made title/og:title and description/og:description in case we want to
> > target
> > > social media differently than search enignes. EG: Providing a description
> with
> > > less than 140 characters for twitter OR providing a more provocative
> > description
> > > for Facebook OR providing a a better targeted description for specific
> search
> > > results. 
> > > You give the word, and I'll remove them, just thought I would explain a
> little
> > > better.
> > 
> > What about updating the logic to something like:
> > {% if description %}
> >   <meta name="description"
content="{{description|translate("description")}}"
> />
> >   {% if og_description %}
> >     <meta property="og:description"
> > content="{{og_description|translate("og_description")}}" />
> >   {% else %}
> >     <meta property="og:description" content="{{description}}" />
> >   {% endif %}
> > {% endif %}
> > 
> > I think same can work for the title, not sure if we are planing to provide
> > og:title separately too often.
> > 
> > We mostly used to have same description for general description and open
> graph,
> > so we need to try not to have duplicated translations, otherwise the
> translation
> > needs to be done for the same string separately
> 
> I don't see the benefit in this change?
> - it breaks the grouping html/open graph/twitter
> - og:description defaults to html5 description by default

Yes you are right, in that case we will need to add them as soon we will use in
the page, so currently I think we only can have a description here and add other
tags as soon we will use them in this conditional fashion as you have presented
here.

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.t...
templates/default.tmpl:35: {% if og_url %}
On 2016/06/01 16:09:50, juliandoucette wrote:
> On 2016/06/01 15:59:28, saroyanm wrote:
> > On 2016/06/01 15:56:38, juliandoucette wrote:
> > > On 2016/06/01 14:38:01, saroyanm wrote:
> > > > This is not being used anywhere, we can add it as soon we will use
og:url,
> > or
> > > > maybe this should be same as our canonical URL ? I'm not really sure
what
> > > og:url
> > > > meta tag used for.
> > > 
> > > It is like a canonical URL. I chose to include it because:
> > > "The four required properties for every page are:" (http://ogp.me/)
> > > 
> > > Something to consider:
> > > http://stackoverflow.com/questions/7830846/ogurl-is-driving-me-crazy
> > > 
> > > I think:
> > > - this is a feature
> > > - this is standard
> > > - this should default to the shared url
> > > 
> > > Thus:
> > > - I have no problem removing it
> > 
> > So maybe in this case we can have something like:
> > <meta property="og:url" content="{{og_url if og_url else
> > https://adblockplus.org/%7B%7Bpage%7D%7D%7D%7D%22 /> ? 
> 
> ~no
> 
> See:
> https://developers.facebook.com/docs/sharing/best-practices#tags
> 
> We could use something like that in the future to handle locale better.
> 
> But, today, we don't have additional query params, so the correct url (unless
we
> specify otherwise) is the default url (the one being shared / the one that
will
> be used if none is provided).

Same goes here as my previous comment, we need to use this to implement in this
review, we can add this condition as soon we will use this condition in the
page, currently we do not use it anywhere, so it's can't be part of this review
unfortunately.

saroyanm

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#newcode18 pages/adware.md:18: margin: 0px 35px 10px 0px; Right margin will misalign ...

June 6, 2016, 2:47 p.m. (2016-06-06 14:47:32 UTC) #16

Philip Hill

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#newcode145 pages/adware.md:145: * [{{malwarebytes-brand Malwarebytes}}]({{malwarebytes-win-affiliate-link http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}) On 2016/06/06 14:47:31, saroyanm wrote: ...

June 6, 2016, 2:52 p.m. (2016-06-06 14:52:51 UTC) #17

saroyanm

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#newcode145 pages/adware.md:145: * [{{malwarebytes-brand Malwarebytes}}]({{malwarebytes-win-affiliate-link http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}) On 2016/06/06 14:52:51, Philip Hill ...

June 6, 2016, 3:44 p.m. (2016-06-06 15:44:52 UTC) #18

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:145: * [{{malwarebytes-brand
Malwarebytes}}]({{malwarebytes-win-affiliate-link
http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}})
On 2016/06/06 14:52:51, Philip Hill wrote:
> On 2016/06/06 14:47:31, saroyanm wrote:
> > As mentioned here:
> >
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> > 
> > 
> > Our CMS is not supporting the link translations. So this links currently are
> not
> > working.
> > So I can see two options here:
> > 1. We are using not localized links and relying to the websites to manage
> that.
> > 2. We will need to hardcode all the links here and hide and show according
to
> > the locale.
> > I'd say that I'll be in favor of solution 1, because going with 2-nd
solution
> it
> > will not be scalable and maintainable and we will need to map their locales
to
> > our. But probably this needs to also be communicated in the ticket as well,
so
> > please let me know what you think about this.
> 
> As I mentioned already, localizing website contents and links is a standard
> problem that we have to solve since we will face this more and more with more
> specific environments that we (legally) move into. Another such example is the
> notification text we show to French users only or the different GPL link for
> French users. Working around a proper solution now will simply delay solving
> this in future where we might be under more time pressure than now.
Having another look into the issue, seems like the reason why the links are
screwed is because of markdown screwing them when we are using our CMS syntax,
so seems like the issue is irrelevant, maybe it still will be possible to
translate through crowdin, make sense to test, just in case, let me investigate
the issue more.

saroyanm

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#newcode145 pages/adware.md:145: * [{{malwarebytes-brand Malwarebytes}}]({{malwarebytes-win-affiliate-link http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}) On 2016/06/06 15:44:52, saroyanm wrote: ...

June 6, 2016, 3:53 p.m. (2016-06-06 15:53:39 UTC) #19

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:145: * [{{malwarebytes-brand
Malwarebytes}}]({{malwarebytes-win-affiliate-link
http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}})
On 2016/06/06 15:44:52, saroyanm wrote:
> On 2016/06/06 14:52:51, Philip Hill wrote:
> > On 2016/06/06 14:47:31, saroyanm wrote:
> > > As mentioned here:
> > >
> >
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> > > 
> > > 
> > > Our CMS is not supporting the link translations. So this links currently
are
> > not
> > > working.
> > > So I can see two options here:
> > > 1. We are using not localized links and relying to the websites to manage
> > that.
> > > 2. We will need to hardcode all the links here and hide and show according
> to
> > > the locale.
> > > I'd say that I'll be in favor of solution 1, because going with 2-nd
> solution
> > it
> > > will not be scalable and maintainable and we will need to map their
locales
> to
> > > our. But probably this needs to also be communicated in the ticket as
well,
> so
> > > please let me know what you think about this.
> > 
> > As I mentioned already, localizing website contents and links is a standard
> > problem that we have to solve since we will face this more and more with
more
> > specific environments that we (legally) move into. Another such example is
the
> > notification text we show to French users only or the different GPL link for
> > French users. Working around a proper solution now will simply delay solving
> > this in future where we might be under more time pressure than now.
> Having another look into the issue, seems like the reason why the links are
> screwed is because of markdown screwing them when we are using our CMS syntax,
> so seems like the issue is irrelevant, maybe it still will be possible to
> translate through crowdin, make sense to test, just in case, let me
investigate
> the issue more.

@Julian probably if you will use <a> tags the links will not be broken, I
thought it was our CMS, but apparently it's Markdown, so I'll suggest to
implement using <a> inline tag instead of markdown syntax for this issue and if
that works I'll create a ticket for Markdown after investigating it, probably
that can be issue on Markdown side, but not sure yet :/ 
* <a href="{{malwarebytes-win-affiliate-link
http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}">M...>

juliandoucette

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl#newcode26 templates/default.tmpl:26: {% if description %} On 2016/06/02 15:05:11, saroyanm wrote: ...

June 13, 2016, 6:12 p.m. (2016-06-13 18:12:51 UTC) #20

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29342978/templates/default.t...
templates/default.tmpl:26: {% if description %}
On 2016/06/02 15:05:11, saroyanm wrote:
> On 2016/06/01 16:23:19, juliandoucette wrote:
> > On 2016/06/01 14:38:01, saroyanm wrote:
> > > On 2016/05/31 19:27:46, juliandoucette wrote:
> > > > On 2016/05/31 15:23:36, saroyanm wrote:
> > > > > I'm not sure if introduction of this conditions are good idea seems
like
> > you
> > > > > only using description variable in adware.md, but not "og_title",
> > "og:type",
> > > > > "og:url", so I think it make sense to only introduce twitter cards
here
> > and
> > > > also
> > > > > without check for the "twitter_card", "twitter_site" and
> "twitter_creator"
> > > > > variables, we will add them when we will use them elsewhere, also
> probably
> > > > > doesn't make sense to devide "title" and "og:title", "description" and
> > > > > "og:description" because mostly I think they should have same content
> and
> > > > > doesn't make sense to translate them separately, anyway we can align
> > > regarding
> > > > > that in separate review, let's leave it for later.
> > > > 
> > > > - I made description conditional because some pages already define it
> inside
> > > of
> > > > their <head>
> > > > - I made title/og:title and description/og:description in case we want
to
> > > target
> > > > social media differently than search enignes. EG: Providing a
description
> > with
> > > > less than 140 characters for twitter OR providing a more provocative
> > > description
> > > > for Facebook OR providing a a better targeted description for specific
> > search
> > > > results. 
> > > > You give the word, and I'll remove them, just thought I would explain a
> > little
> > > > better.
> > > 
> > > What about updating the logic to something like:
> > > {% if description %}
> > >   <meta name="description"
> content="{{description|translate("description")}}"
> > />
> > >   {% if og_description %}
> > >     <meta property="og:description"
> > > content="{{og_description|translate("og_description")}}" />
> > >   {% else %}
> > >     <meta property="og:description" content="{{description}}" />
> > >   {% endif %}
> > > {% endif %}
> > > 
> > > I think same can work for the title, not sure if we are planing to provide
> > > og:title separately too often.
> > > 
> > > We mostly used to have same description for general description and open
> > graph,
> > > so we need to try not to have duplicated translations, otherwise the
> > translation
> > > needs to be done for the same string separately
> > 
> > I don't see the benefit in this change?
> > - it breaks the grouping html/open graph/twitter
> > - og:description defaults to html5 description by default
> 
> Yes you are right, in that case we will need to add them as soon we will use
in
> the page, so currently I think we only can have a description here and add
other
> tags as soon we will use them in this conditional fashion as you have
presented
> here.

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:18: margin: 0px 35px 10px 0px;
On 2016/06/06 14:47:31, saroyanm wrote:
> Right margin will misalign the element for RTL direction. I think we can
> increase the width of the first element, or remove right margin.

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:65: margin-left: 0px;
On 2016/06/06 14:47:30, saroyanm wrote:
> Please also consider RTL direction.

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:87: .list-custom ul {
On 2016/06/06 14:47:30, saroyanm wrote:
> Detail: the braces should go to their own line

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:105: .list-custom .check-item
On 2016/06/06 14:47:31, saroyanm wrote:
> This solution also doesn't work for RTL directions.
> 
> This probably will need to be separate element, maybe:
> li.check-item::before
> {
>   content: "";
>   display: inline-block;
>   height: 17px;
>   width: 17px;
>   background-image: url(/img/check.png);
> }
> But probably in that case we will need to wrap also the content <span> should
> work I guess.

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:145: * [{{malwarebytes-brand
Malwarebytes}}]({{malwarebytes-win-affiliate-link
http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}})
On 2016/06/06 15:53:39, saroyanm wrote:
> On 2016/06/06 15:44:52, saroyanm wrote:
> > On 2016/06/06 14:52:51, Philip Hill wrote:
> > > On 2016/06/06 14:47:31, saroyanm wrote:
> > > > As mentioned here:
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n...
> > > > 
> > > > 
> > > > Our CMS is not supporting the link translations. So this links currently
> are
> > > not
> > > > working.
> > > > So I can see two options here:
> > > > 1. We are using not localized links and relying to the websites to
manage
> > > that.
> > > > 2. We will need to hardcode all the links here and hide and show
according
> > to
> > > > the locale.
> > > > I'd say that I'll be in favor of solution 1, because going with 2-nd
> > solution
> > > it
> > > > will not be scalable and maintainable and we will need to map their
> locales
> > to
> > > > our. But probably this needs to also be communicated in the ticket as
> well,
> > so
> > > > please let me know what you think about this.
> > > 
> > > As I mentioned already, localizing website contents and links is a
standard
> > > problem that we have to solve since we will face this more and more with
> more
> > > specific environments that we (legally) move into. Another such example is
> the
> > > notification text we show to French users only or the different GPL link
for
> > > French users. Working around a proper solution now will simply delay
solving
> > > this in future where we might be under more time pressure than now.
> > Having another look into the issue, seems like the reason why the links are
> > screwed is because of markdown screwing them when we are using our CMS
syntax,
> > so seems like the issue is irrelevant, maybe it still will be possible to
> > translate through crowdin, make sense to test, just in case, let me
> investigate
> > the issue more.
> 
> @Julian probably if you will use <a> tags the links will not be broken, I
> thought it was our CMS, but apparently it's Markdown, so I'll suggest to
> implement using <a> inline tag instead of markdown syntax for this issue and
if
> that works I'll create a ticket for Markdown after investigating it, probably
> that can be issue on Markdown side, but not sure yet :/ 
> * <a href="{{malwarebytes-win-affiliate-link
>
http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial%7D%7...>

Done.

I vote we investigate this issue further in a separate CMS issue.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:168: ### <span class="badge badge-success">2</span>
{{reset-browser-settings-heading Reset your browser settings}} { .split-section
.badge-header }
On 2016/06/06 14:47:31, saroyanm wrote:
> Class "badge" is not being used anywhere

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/pages/adware.md#new...
pages/adware.md:192: * **{{install-abp-heading Install Adblock Plus.}}**
{{install-abp-content Adblock Plus can help to block and hide ads that trick you
into installing potentially unwanted programs. Get Adblock Plus from}}
[https://adblockplus.org/](https://adblockplus.org/).
On 2016/06/06 14:47:30, saroyanm wrote:
> The product name can be fixed: <fix>Adblock Plus</fix>
> and translations can be included like "some text {1} some text"
> Apparently this is not documented in the Readme :/

Is it obvious for translators that the fixed text is our brand name?

Done.

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.t...
templates/default.tmpl:35: {% if og_url %}
On 2016/06/02 15:05:11, saroyanm wrote:
> On 2016/06/01 16:09:50, juliandoucette wrote:
> > On 2016/06/01 15:59:28, saroyanm wrote:
> > > On 2016/06/01 15:56:38, juliandoucette wrote:
> > > > On 2016/06/01 14:38:01, saroyanm wrote:
> > > > > This is not being used anywhere, we can add it as soon we will use
> og:url,
> > > or
> > > > > maybe this should be same as our canonical URL ? I'm not really sure
> what
> > > > og:url
> > > > > meta tag used for.
> > > > 
> > > > It is like a canonical URL. I chose to include it because:
> > > > "The four required properties for every page are:" (http://ogp.me/)
> > > > 
> > > > Something to consider:
> > > > http://stackoverflow.com/questions/7830846/ogurl-is-driving-me-crazy
> > > > 
> > > > I think:
> > > > - this is a feature
> > > > - this is standard
> > > > - this should default to the shared url
> > > > 
> > > > Thus:
> > > > - I have no problem removing it
> > > 
> > > So maybe in this case we can have something like:
> > > <meta property="og:url" content="{{og_url if og_url else
> > > https://adblockplus.org/%7B%7Bpage%7D%7D%7D%7D%22 /> ? 
> > 
> > ~no
> > 
> > See:
> > https://developers.facebook.com/docs/sharing/best-practices#tags
> > 
> > We could use something like that in the future to handle locale better.
> > 
> > But, today, we don't have additional query params, so the correct url
(unless
> we
> > specify otherwise) is the default url (the one being shared / the one that
> will
> > be used if none is provided).
> 
> Same goes here as my previous comment, we need to use this to implement in
this
> review, we can add this condition as soon we will use this condition in the
> page, currently we do not use it anywhere, so it's can't be part of this
review
> unfortunately.

Done.

saroyanm

Some small changes, we are close I guess. https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29340844/diff/29345439/templates/default.tmpl#newcode44 templates/default.tmpl:44: <meta ...

June 21, 2016, 2:58 p.m. (2016-06-21 14:58:05 UTC) #21

saroyanm

https://codereview.adblockplus.org/29340844/diff/29345778/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29345778/pages/adware.md#newcode187 pages/adware.md:187: * <a href="{{adcleaner-win-affiliate-link https://toolslib.net/downloads/viewdownload/1-adwcleaner/}}">{{adwcleaner-brand AdwCleaner}}</a> Apparently we only need ...

June 23, 2016, 8:45 a.m. (2016-06-23 08:45:34 UTC) #22

juliandoucette

Fixed issues in review. I also discovered an issue in the generated HTML. If you ...

June 23, 2016, 4:11 p.m. (2016-06-23 16:11:01 UTC) #23

juliandoucette

On 2016/06/23 16:11:01, juliandoucette wrote: > Fixed issues in review. > > I also discovered ...

June 23, 2016, 4:19 p.m. (2016-06-23 16:19:46 UTC) #24

juliandoucette

On 2016/06/23 16:19:46, juliandoucette wrote: > On 2016/06/23 16:11:01, juliandoucette wrote: > > Fixed issues ...

June 23, 2016, 4:56 p.m. (2016-06-23 16:56:39 UTC) #25

saroyanm

Not Just some CSS improvements, this is small changes, but should make the styles more ...

June 27, 2016, 5:12 p.m. (2016-06-27 17:12:32 UTC) #26

Not Just some CSS improvements, this is small changes, but should make the
styles more readable I think.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:51: .badge-success
I think you can use ".badge-header span" selector here and remove the
".badge-success" from element while .badge-success is always related to the
".badge-header".

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:67: .list-custom,
Seems like this applies to all "UL" elements on the page, so you can use "ul" as
a selector here:
ul
{
  margin-top: 10px;
  margin-bottom: 10px;
  padding: 0px;
}

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:76: .list-custom ul
.list-custom, doesn't say anything about the style, so I'll subbest to change
the name to "badged" for example, so that will describe that it's holding the
"check" and "cross" "icons".

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:84: .list-custom ul li
I think while we have specific style ".list-unstyled" it's better to apply the
style to the ".list-custom ul" element I think while it's just one element.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:92: .list-unstyled li,
This selectors can be merged with the one below, also looks like this applies to
all "li"s:
ul li
{
  list-style-type: none;
  padding-bottom: 10px;
  line-height: 20px;
}

Also this can be moved below "ul" rule to make styles more readable.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:114: [dir="ltr"] .list-custom
No need to mention "ltr" direction specifically, this style will apply by
default regardless of the direction.
Same goes to user "ltr" specific styles.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:129: .list-custom .check-item,
All .check-item and .cross-item are inside of the ".list-custom" elements, so I
think we do not need specifically mention them and also you should be able to
get rid of the one selectors below:
.check-item,
.cross-item
{
  position: absolute;
  left: 0px; (we do not need to specify right direction while it's default value
is auto)
  height: 20px;
  width: 40px;
}

[dir="rtl"] .check-item,
[dir="rtl"] .cross-item
{
  right: 0px;
}

.check-item
{
  background: url("/img/check.png") no-repeat center top;
}

.cross-item
{
 background: url("/img/cross.png") no-repeat center top;
}

Alternatively you can .icon.cross and .icon.check, so you can have one selector
(.icon) for the styles above, which will be more generic for this page.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:192: * <a href="{{hitmanpro-win-affiliate-link
http://www.surfright.nl/en/hitmanpro}}">HitmanPro</a>
As mentioned here
"https://codereview.adblockplus.org/29340844/diff/29345778/pages/adware.md#newcode187",
we need to only translate the malwarebytes links. Same goes to other non
malwarebyte links.

saroyanm

Just some CSS improvements, this is small changes, but should make the styles more readable ...

June 27, 2016, 5:13 p.m. (2016-06-27 17:13:54 UTC) #27

juliandoucette

Good suggestions Manvel :) https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#newcode51 pages/adware.md:51: .badge-success On 2016/06/27 17:12:31, saroyanm ...

June 30, 2016, 7:05 p.m. (2016-06-30 19:05:55 UTC) #28

Good suggestions Manvel :)

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:51: .badge-success
On 2016/06/27 17:12:31, saroyanm wrote:
> I think you can use ".badge-header span" selector here and remove the
> ".badge-success" from element while .badge-success is always related to the
> ".badge-header".

Done.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:67: .list-custom,
On 2016/06/27 17:12:30, saroyanm wrote:
> Seems like this applies to all "UL" elements on the page, so you can use "ul"
as
> a selector here:
> ul
> {
>   margin-top: 10px;
>   margin-bottom: 10px;
>   padding: 0px;
> }

Done.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:76: .list-custom ul
On 2016/06/27 17:12:31, saroyanm wrote:
> .list-custom, doesn't say anything about the style, so I'll subbest to change
> the name to "badged" for example, so that will describe that it's holding the
> "check" and "cross" "icons".

Done.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:84: .list-custom ul li
On 2016/06/27 17:12:29, saroyanm wrote:
> I think while we have specific style ".list-unstyled" it's better to apply the
> style to the ".list-custom ul" element I think while it's just one element.

This style is used in several other places outside of .list-custom

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:84: .list-custom ul li
On 2016/06/27 17:12:29, saroyanm wrote:
> I think while we have specific style ".list-unstyled" it's better to apply the
> style to the ".list-custom ul" element I think while it's just one element.

Since these two styles cover all of the ul elements on this page, we can change
it to #content ul, #content li

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:92: .list-unstyled li,
On 2016/06/27 17:12:30, saroyanm wrote:
> This selectors can be merged with the one below, also looks like this applies
to
> all "li"s:
> ul li
> {
>   list-style-type: none;
>   padding-bottom: 10px;
>   line-height: 20px;
> }
> 
> Also this can be moved below "ul" rule to make styles more readable.

Done.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:114: [dir="ltr"] .list-custom
On 2016/06/27 17:12:30, saroyanm wrote:
> No need to mention "ltr" direction specifically, this style will apply by
> default regardless of the direction.
> Same goes to user "ltr" specific styles.

Done.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:129: .list-custom .check-item,
On 2016/06/27 17:12:30, saroyanm wrote:
> All .check-item and .cross-item are inside of the ".list-custom" elements, so
I
> think we do not need specifically mention them and also you should be able to
> get rid of the one selectors below:
> .check-item,
> .cross-item
> {
>   position: absolute;
>   left: 0px; (we do not need to specify right direction while it's default
value
> is auto)
>   height: 20px;
>   width: 40px;
> }
> 
> [dir="rtl"] .check-item,
> [dir="rtl"] .cross-item
> {
>   right: 0px;
> }
> 
> .check-item
> {
>   background: url("/img/check.png") no-repeat center top;
> }
> 
> .cross-item
> {
>  background: url("/img/cross.png") no-repeat center top;
> }
> 
> Alternatively you can .icon.cross and .icon.check, so you can have one
selector
> (.icon) for the styles above, which will be more generic for this page. 

Good idea.

I'll do you 1 better though. We can use .icon-list > li > span.

https://codereview.adblockplus.org/29340844/diff/29346989/pages/adware.md#new...
pages/adware.md:192: * <a href="{{hitmanpro-win-affiliate-link
http://www.surfright.nl/en/hitmanpro}}">HitmanPro</a>
On 2016/06/27 17:12:29, saroyanm wrote:
> As mentioned here
>
"https://codereview.adblockplus.org/29340844/diff/29345778/pages/adware.md#newcode187",
> we need to only translate the malwarebytes links. Same goes to other non
> malwarebyte links.

Done.

saroyanm

4 Small comments and I think we are ready to go! https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md File pages/adware.md (right): ...

July 4, 2016, 5:38 p.m. (2016-07-04 17:38:06 UTC) #29

juliandoucette

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#newcode32 pages/adware.md:32: .text-danger On 2016/07/04 17:38:05, saroyanm wrote: > Detail: What ...

July 4, 2016, 6:07 p.m. (2016-07-04 18:07:16 UTC) #30

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#new...
pages/adware.md:32: .text-danger
On 2016/07/04 17:38:05, saroyanm wrote:
> Detail: What about having a container name as "warning", so you will not need
to
> assing separate styles to the items, something like:
> .warning
> {
>   color: #e11a2c;
> }
> .warning p
> {
>   padding: 15px;
>   border: 2px solid;
>   border-color: #e11a2c;
>   background-color: #fff;
> }

The original intention was to create these classes in a general way such that
they could integrate into main.css. Thus, a class for _just making red text_
made sense.

If we want to take the opposite approach then your suggestion is valid. I will
implement it.

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#new...
pages/adware.md:112: left: 0;
On 2016/07/04 17:38:05, saroyanm wrote:
> Detail: CSS number values should specify units where possible, same goes to
the
> one below

Done.

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#new...
pages/adware.md:165: * <a href="http://www.surfright.nl/hitmanpro">HitmanPro</a>
On 2016/07/04 17:38:05, saroyanm wrote:
> Detail:
> Please use the markdown where its possible:
> * [HitmanPro](http://www.surfright.nl/hitmanpro/)
> * [AdwCleaner](https://toolslib.net/downloads/viewdownload/1-adwcleaner/)

Done.

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#new...
pages/adware.md:204: * <span class="check-item"></span>
**{{use-official-channels-heading Use official channels to download your
browser:}}**
On 2016/07/04 17:38:06, saroyanm wrote:
> What about having a class on list item, so you can replace HTML implementation
> ex.:
> * **{{use-official-channels-heading Use official channels to download your
> browser:}}**
> {.icon.check}
> 
> .icon::before
> {
>   content: "";
>   position: absolute;
>   left: 0;
>   right: auto;
>   height: 20px;
>   width: 40px;
> }
> 
> .check::before
> {
>   background: url("/img/check.png") no-repeat center top;
> }
> 
> .cross::before
> {
>   background: url("/img/cross.png") no-repeat center top;
> }

Good catch.

I'll do you one better. Since each list item in an .icon-list contains an .icon,
we don't need the .icon class anymore. .icon-list > li makes more sense.

saroyanm

LGTM https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#newcode32 pages/adware.md:32: .text-danger On 2016/07/04 18:07:15, juliandoucette wrote: > On ...

July 4, 2016, 6:36 p.m. (2016-07-04 18:36:15 UTC) #32

LGTM

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md
File pages/adware.md (right):

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#new...
pages/adware.md:32: .text-danger
On 2016/07/04 18:07:15, juliandoucette wrote:
> On 2016/07/04 17:38:05, saroyanm wrote:
> > Detail: What about having a container name as "warning", so you will not
need
> to
> > assing separate styles to the items, something like:
> > .warning
> > {
> >   color: #e11a2c;
> > }
> > .warning p
> > {
> >   padding: 15px;
> >   border: 2px solid;
> >   border-color: #e11a2c;
> >   background-color: #fff;
> > }
> 
> The original intention was to create these classes in a general way such that
> they could integrate into main.css. Thus, a class for _just making red text_
> made sense.
> 
> If we want to take the opposite approach then your suggestion is valid. I will
> implement it.

In future if we will use it more often yes we will need to make it more generic,
currently I'm not sure if this will be used anywhere else, but yes in case we
will start to use it often will use current approach or modify it. Anyway we can
make it more generic while not creating two separate classes that will apply to
the related to each other "Parent" and "child".

https://codereview.adblockplus.org/29340844/diff/29347205/pages/adware.md#new...
pages/adware.md:204: * <span class="check-item"></span>
**{{use-official-channels-heading Use official channels to download your
browser:}}**
On 2016/07/04 18:07:15, juliandoucette wrote:
> On 2016/07/04 17:38:06, saroyanm wrote:
> > What about having a class on list item, so you can replace HTML
implementation
> > ex.:
> > * **{{use-official-channels-heading Use official channels to download your
> > browser:}}**
> > {.icon.check}
> > 
> > .icon::before
> > {
> >   content: "";
> >   position: absolute;
> >   left: 0;
> >   right: auto;
> >   height: 20px;
> >   width: 40px;
> > }
> > 
> > .check::before
> > {
> >   background: url("/img/check.png") no-repeat center top;
> > }
> > 
> > .cross::before
> > {
> >   background: url("/img/cross.png") no-repeat center top;
> > }
> 
> Good catch.
> 
> I'll do you one better. Since each list item in an .icon-list contains an
.icon,
> we don't need the .icon class anymore. .icon-list > li makes more sense.

I thought it's more readable, when you have as a class "icon" and "check" or
"cross", which make it more intentional for user writing Markdown to use, but
this implementation still looks fine for me, my initial intention was to get rid
of the HTML tags.

juliandoucette

We are also waiting on a final word from Tamara about fixing and trademarking brand ...

July 4, 2016, 7:30 p.m. (2016-07-04 19:30:07 UTC) #34

saroyanm

Okey, more LGTM implementation wise from my side, let's wait for final go from Tamara ...

July 5, 2016, 7:50 a.m. (2016-07-05 07:50:27 UTC) #35

tamara

On 2016/07/05 07:50:27, saroyanm wrote: > Okey, more LGTM implementation wise from my side, let's ...

July 7, 2016, 9:45 a.m. (2016-07-07 09:45:09 UTC) #36

saroyanm

https://codereview.adblockplus.org/29340844/diff/29347291/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29347291/pages/adware.md#newcode162 pages/adware.md:162: * <a href="{{malwarebytes-win-affiliate-link http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}">Malwarebytes</a> @Julian: before pushing the changes ...

July 7, 2016, 1:02 p.m. (2016-07-07 13:02:27 UTC) #37

juliandoucette

https://codereview.adblockplus.org/29340844/diff/29347291/pages/adware.md File pages/adware.md (right): https://codereview.adblockplus.org/29340844/diff/29347291/pages/adware.md#newcode162 pages/adware.md:162: * <a href="{{malwarebytes-win-affiliate-link http://buy.malwarebytes.com/get-trial/adblock/en/?c=adblock&s=en&k=trial}}">Malwarebytes</a> On 2016/07/07 13:02:26, saroyanm wrote: ...

July 7, 2016, 8:26 p.m. (2016-07-07 20:26:55 UTC) #38

juliandoucette

> So, by the information I've managed to gather (wasn't easy), Google Chrome is > ...

July 14, 2016, 5:32 p.m. (2016-07-14 17:32:12 UTC) #39

tamara

On 2016/07/14 17:32:12, juliandoucette wrote: > > So, by the information I've managed to gather ...

July 15, 2016, 8:29 a.m. (2016-07-15 08:29:41 UTC) #40

juliandoucette

> > Am I still waiting for you Tamara? > > :-/ Sorry! I didn't ...

July 15, 2016, 1:03 p.m. (2016-07-15 13:03:29 UTC) #41

tamara

On 2016/07/15 13:03:29, juliandoucette wrote: > > > Am I still waiting for you Tamara? ...

July 15, 2016, 1:26 p.m. (2016-07-15 13:26:03 UTC) #42

juliandoucette

On 2016/07/15 13:26:03, tamara wrote: > On 2016/07/15 13:03:29, juliandoucette wrote: > > > > ...

July 15, 2016, 2:14 p.m. (2016-07-15 14:14:29 UTC) #43

tamara

On 2016/07/15 14:14:29, juliandoucette wrote: > On 2016/07/15 13:26:03, tamara wrote: > > On 2016/07/15 ...

July 18, 2016, 8:33 a.m. (2016-07-18 08:33:05 UTC) #44

juliandoucette

> Then in that case I should imagine you can go ahead. ^^ Do you ...

July 19, 2016, 8:41 p.m. (2016-07-19 20:41:10 UTC) #45

juliandoucette

Aug. 22, 2016, 12:29 p.m. (2016-08-22 12:29:05 UTC) #46

https://hg.adblockplus.org/web.adblockplus.org/rev/eb77ecac6ccb

Issue 29340844: Issue 3802 - Create page to inform users about adware on web.adblockplus.org (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fixed twitter handle, images, and styles. #

Patch Set 3 : Forgot to remove hr-top classes #

Patch Set 4 : moved meta tags, re-wrote in markdown, moved css, optimized images #

Patch Set 5 : Fixed meta translations #

Patch Set 6 : Fixed styles, moved styles back to page, fixed translation error #

Patch Set 7 : Removed meta tags, fixed rtl, fixed affiliate links #

Patch Set 8 : Fixed rtl .custom-list spacing #

Patch Set 9 : Fixed brand labels, added descriptions to translated meta strings #

Patch Set 10 : Fixed zombie list item #

Patch Set 11 : Changed a couple class names to be more descriptive and reduced nested list padding #

Patch Set 12 : Fixed CSS NITs #

Patch Set 13 : Refactored markdown and styles #

Patch Set 14 : Forgot red text in icon list #

Patch Set 15 : Refactored away obsolute HTML tags in markdown #

Messages