Issue 5693 - Add Article Content to help.eyeo.com

Issue 5693 - Add Article Content to help.eyeo.com

[ire, 2017-09-20 08:18:03]

Ready for review. See my comments below

https://codereview.adblockplus.org/29550574/diff/29550575/includes/adblockplu...
File includes/adblockplus/block-all-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29550575/includes/adblockplu...
includes/adblockplus/block-all-ads.md:1: <div class="platform-ios" markdown="1">
The idea here is that I will hide/show the content based on the browser that is
currently selected.

https://codereview.adblockplus.org/29550574/diff/29550575/pages/adblockplus/a...
File pages/adblockplus/add-a-filter-list.md (right):

https://codereview.adblockplus.org/29550574/diff/29550575/pages/adblockplus/a...
pages/adblockplus/add-a-filter-list.md:14: </div>
I'm not sure what to do for cases where there is no content for the specific
browser. We may have to have a meeting to discuss how we want the browser
detection & displaying of content to work. 

There are 2 ways I think it could work:

1. The platform selector shows all platforms we support, and by default will
always be selected to the user's current browser. If there is no content for
that browser, there could be a message saying something along the lines of "No
content available for this browser" (but better worded)

or

2. The platform selector will only show platforms that the current article has
content for. If the current user isn't in one of those browsers, they are either
shown all the content, or the platform selector will default to a different
browser that the current article does have content. 

--

This decision affects if there should still be a <div> for platforms that don't
have content for the current article.

https://codereview.adblockplus.org/29550574/diff/29550575/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29550574/diff/29550575/templates/article.t...
templates/article.tmpl:1: {% extends "templates/default" %}
I created this file for the purpose of testing the pages. I will probably
exclude it from the commit since this is just about adding the content.

[juliandoucette, 2017-09-22 14:42:14]

Responses before review.

https://codereview.adblockplus.org/29550574/diff/29550575/includes/adblockplu...
File includes/adblockplus/block-all-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29550575/includes/adblockplu...
includes/adblockplus/block-all-ads.md:1: <div class="platform-ios" markdown="1">
On 2017/09/20 08:18:03, ire wrote:
> The idea here is that I will hide/show the content based on the browser that
is
> currently selected.

Acknowledged.

SuperNIT/Suggest: $platform-only

https://codereview.adblockplus.org/29550574/diff/29550575/pages/adblockplus/a...
File pages/adblockplus/add-a-filter-list.md (right):

https://codereview.adblockplus.org/29550574/diff/29550575/pages/adblockplus/a...
pages/adblockplus/add-a-filter-list.md:14: </div>
On 2017/09/20 08:18:03, ire wrote:
> I'm not sure what to do for cases where there is no content for the specific
> browser. We may have to have a meeting to discuss how we want the browser
> detection & displaying of content to work. 
> 
> There are 2 ways I think it could work:
> 
> 1. The platform selector shows all platforms we support, and by default will
> always be selected to the user's current browser. If there is no content for
> that browser, there could be a message saying something along the lines of "No
> content available for this browser" (but better worded)
> 
> or
> 
> 2. The platform selector will only show platforms that the current article has
> content for. If the current user isn't in one of those browsers, they are
either
> shown all the content, or the platform selector will default to a different
> browser that the current article does have content. 
> 
> --
> 
> This decision affects if there should still be a <div> for platforms that
don't
> have content for the current article. 

This is a tough question.

I think we should:

1. Show a friendly message if the platform is missing
2. List available platforms in meta data so that we can add them to list item
classes and hide them in topic lists

I think we should run this by Jeen.

https://codereview.adblockplus.org/29550574/diff/29550575/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29550574/diff/29550575/templates/article.t...
templates/article.tmpl:1: {% extends "templates/default" %}
On 2017/09/20 08:18:03, ire wrote:
> I created this file for the purpose of testing the pages. I will probably
> exclude it from the commit since this is just about adding the content.

Acknowledged.

[juliandoucette, 2017-09-22 14:49:25]

Do you want me to review anything else before proceeding?

[ire, 2017-09-22 14:54:31]

On 2017/09/22 14:49:25, juliandoucette wrote:
> Do you want me to review anything else before proceeding?

Nope, you can proceed. Thanks.

[juliandoucette, 2017-09-22 18:08:03]

On 2017/09/22 14:54:31, ire wrote:
> On 2017/09/22 14:49:25, juliandoucette wrote:
> > Do you want me to review anything else before proceeding?
> 
> Nope, you can proceed. Thanks.

I'm not sure if we understand each other. If I understand correctly, I am not
going to review this until you upload a new Patchset.

[ire, 2017-09-25 07:04:51]

On 2017/09/22 18:08:03, juliandoucette wrote:
> On 2017/09/22 14:54:31, ire wrote:
> > On 2017/09/22 14:49:25, juliandoucette wrote:
> > > Do you want me to review anything else before proceeding?
> > 
> > Nope, you can proceed. Thanks.
> 
> I'm not sure if we understand each other. If I understand correctly, I am not
> going to review this until you upload a new Patchset.

Sorry my response was confusing. I was asking that you do continue with the
review. From your responses, the main thing that needs to be changed is issue of
how we handle cases where the article doesn't have content for all platforms. I
think you can continue the review while I discuss with Jeen on this particular
issue.

[ire, 2017-09-25 08:51:48]

UPDATE: Jeen agrees with us and prefers the solution of displaying a message to
users if there is no content for them. I'm waiting on her to get back to me with
the message that should be displayed. You can wait for me to update with a new
patchset if you have higher priority things to do.

[ire, 2017-09-26 07:16:02]

> 1. Show a friendly message if the platform is missing

Done. I have asked Jeen but she has not gotten back to me on the message that
should be displayed, so I wrote one myself for the meantime.

> 2. List available platforms in meta data so that we can add them to list item
classes and hide them in topic lists

I'm not sure I understand what you are suggesting here. Why would we hide
articles in the topic lists? Even if the current browser the user is on doesn't
have any related articles, they may not be looking for content for that browser.

[ire, 2017-09-26 07:17:00]

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/w...
File pages/adblockplus/what-are-acceptable-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/w...
pages/adblockplus/what-are-acceptable-ads.md:7: hide_browser_selector=true
For articles without browser-specific content, I will hide the browser selector
on the article page.

[juliandoucette, 2017-10-09 16:41:06]

TOL = Thinking Out Loud (I don't know if I just made that up)

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/block-all-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:1: <div class="platform-ios" markdown="1">
NIT: I think that these should be <section>s of an <article> or <article>s in a
<section>. One could argue that only the parent <section>/<article> is relevant
because we only show one platform. But I think that is incorrect because we show
all platforms by default (robots, no-js).

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:11: <div class="platform-chrome"
markdown="1">
NIT: You didn't agree/disagree with my NIT/Suggestion of the class name
"${platform}-only"

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:71: <br>The *Adblock Plus Options* tab
opens.
NIT / TOL: It seems like we should decide whether to put <br> on the the same
line or newlines. I personally like the same line. But I'm guessing you didn't
actually write this - and it would be too much trouble to change these.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/remove-a-website-from-the-whitelist.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:2: ## Adblock Plus
for iOS
NIT/TOL: Does h2 or h3 match your implementation?

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:96: <? include
no-content-for-platform-message ?>
I think that we should include this message inside a <template> to be rendered
if your browser is detected and there is not a relevant section on the page;
assuming that all sections are shown by default (no-js, robots) and sections
without instructions will be omitted.

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
File pages/adblockplus/i-still-see-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:3: template=article
NIT: It seems that most pages will be articles. Why not make article the default
template?

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:4: product_id=abp
NIT: Why not *just* "product"?

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:5: category=Troubleshooting & Reporting
NIT: Why not simplify categories by name e.g. "troubleshooting"?

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/w...
File pages/adblockplus/what-are-acceptable-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/w...
pages/adblockplus/what-are-acceptable-ads.md:7: hide_browser_selector=true
On 2017/09/26 07:17:00, ire wrote:
> For articles without browser-specific content, I will hide the browser
selector
> on the article page.

Why not add the browser selector via JavaScript based on whether or not there
are browser specific sections?

[ire, 2017-10-09 20:20:48]

Thanks Julian! New patch uploaded

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/block-all-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:1: <div class="platform-ios" markdown="1">
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT: I think that these should be <section>s of an <article> or <article>s in
a
> <section>. One could argue that only the parent <section>/<article> is
relevant
> because we only show one platform. But I think that is incorrect because we
show
> all platforms by default (robots, no-js).

I think <section>s in an article makes the most sense.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:1: <div class="platform-ios" markdown="1">
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT: I think that these should be <section>s of an <article> or <article>s in
a
> <section>. One could argue that only the parent <section>/<article> is
relevant
> because we only show one platform. But I think that is incorrect because we
show
> all platforms by default (robots, no-js).

Done.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:11: <div class="platform-chrome"
markdown="1">
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT: You didn't agree/disagree with my NIT/Suggestion of the class name
> "${platform}-only"

Sorry apparently I replied in my mind :p

I prefer to stick with "platform-${platform}" because of the cases where the
there are multiple of the classes being applied (e.g. a single <section> with
content for multiple platforms). This just makes more sense to me

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:71: <br>The *Adblock Plus Options* tab
opens.
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT / TOL: It seems like we should decide whether to put <br> on the the same
> line or newlines. I personally like the same line. But I'm guessing you didn't
> actually write this - and it would be too much trouble to change these.

Yes I didn't write this. I personally prefer the <br> to be placed at the end of
the same line, but I might be too lazy to go through and change everything.
You're right that we should decide and communicate this to the content writers
so they can do it that way in the future. Then we can (in another issue) go
through these ones and clean up.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/remove-a-website-from-the-whitelist.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:2: ## Adblock Plus
for iOS
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT/TOL: Does h2 or h3 match your implementation?

h2 is correct. The article title will be h1

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:96: <? include
no-content-for-platform-message ?>
On 2017/10/09 16:41:05, juliandoucette wrote:
> I think that we should include this message inside a <template> to be rendered
> if your browser is detected and there is not a relevant section on the page;
> assuming that all sections are shown by default (no-js, robots) and sections
> without instructions will be omitted.

Could you please expand on this. What sort of <template> are you referring to?

You're definitely right that the no-js state here is not optimal.

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
File pages/adblockplus/i-still-see-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:3: template=article
On 2017/10/09 16:41:06, juliandoucette wrote:
> NIT: It seems that most pages will be articles. Why not make article the
default
> template?

I assume that you mean that the content of the template called `default.tmpl`
should be the article template, because I do not know if it's possible to make
the "default" template something other than a template named "default". If this
is the case, I would prefer to keep it this way. In my mind the "default"
template should sort of be a base, and not really specific to a certain kind of
page.

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:4: product_id=abp
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT: Why not *just* "product"?

I feel like the "product" should be the object that has all the information.
Whereas this is specifically just the id. Also, since I'm using the name
"product" in the article template, which references the full object, there would
be a clash

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:5: category=Troubleshooting & Reporting
On 2017/10/09 16:41:05, juliandoucette wrote:
> NIT: Why not simplify categories by name e.g. "troubleshooting"?

I just felt it would be easier for a content person to write the actual name of
the category, than to know what the simplified category name was.

[juliandoucette, 2017-10-09 22:41:32]

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/block-all-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:1: <div class="platform-ios" markdown="1">
On 2017/10/09 20:20:46, ire wrote:
> I think <section>s in an article makes the most sense. 

I agree.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:11: <div class="platform-chrome"
markdown="1">
On 2017/10/09 20:20:46, ire wrote:
> On 2017/10/09 16:41:05, juliandoucette wrote:
> > NIT: You didn't agree/disagree with my NIT/Suggestion of the class name
> > "${platform}-only"
> 
> Sorry apparently I replied in my mind :p
> 
> I prefer to stick with "platform-${platform}" because of the cases where the
> there are multiple of the classes being applied (e.g. a single <section> with
> content for multiple platforms). This just makes more sense to me

Acknowledged.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/block-all-ads.md:71: <br>The *Adblock Plus Options* tab
opens.
On 2017/10/09 20:20:47, ire wrote:
> On 2017/10/09 16:41:05, juliandoucette wrote:
> > NIT / TOL: It seems like we should decide whether to put <br> on the the
same
> > line or newlines. I personally like the same line. But I'm guessing you
didn't
> > actually write this - and it would be too much trouble to change these.
> 
> Yes I didn't write this. I personally prefer the <br> to be placed at the end
of
> the same line, but I might be too lazy to go through and change everything.
> You're right that we should decide and communicate this to the content writers
> so they can do it that way in the future. Then we can (in another issue) go
> through these ones and clean up.

Acknowledged.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/remove-a-website-from-the-whitelist.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:2: ## Adblock Plus
for iOS
On 2017/10/09 20:20:47, ire wrote:
> On 2017/10/09 16:41:05, juliandoucette wrote:
> > NIT/TOL: Does h2 or h3 match your implementation?
> 
> h2 is correct. The article title will be h1

Acknowledged.

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:96: <? include
no-content-for-platform-message ?>
On 2017/10/09 20:20:47, ire wrote:
> Could you please expand on this. What sort of <template> are you referring to?

Show all available instructions by default; // **no-js**

if (the user's platform can be detected) { // **js-only**
    hide instructions that do not match the user's platform;
    if (there are no instructions for the users platform) {
        show the error message;
    }
}

Place the error message inside of a <script type="text/template"> or <template>
[so that it isn't shown, so that it is not mistaken for content, so that
JavaScript doesn't have to be aware of it's translation].

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
File pages/adblockplus/i-still-see-ads.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:3: template=article
On 2017/10/09 20:20:47, ire wrote:
> On 2017/10/09 16:41:06, juliandoucette wrote:
> > NIT: It seems that most pages will be articles. Why not make article the
> default
> > template?
> 
> I assume that you mean that the content of the template called `default.tmpl`
> should be the article template, because I do not know if it's possible to make
> the "default" template something other than a template named "default". If
this
> is the case, I would prefer to keep it this way. In my mind the "default"
> template should sort of be a base, and not really specific to a certain kind
of
> page.

Correct. And I agree with your preference.

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:4: product_id=abp
On 2017/10/09 20:20:48, ire wrote:
> On 2017/10/09 16:41:05, juliandoucette wrote:
> > NIT: Why not *just* "product"?
> 
> I feel like the "product" should be the object that has all the information.
> Whereas this is specifically just the id. Also, since I'm using the name
> "product" in the article template, which references the full object, there
would
> be a clash

Acknowledged.

https://codereview.adblockplus.org/29550574/diff/29556555/pages/adblockplus/i...
pages/adblockplus/i-still-see-ads.md:5: category=Troubleshooting & Reporting
On 2017/10/09 20:20:48, ire wrote:
> On 2017/10/09 16:41:05, juliandoucette wrote:
> > NIT: Why not simplify categories by name e.g. "troubleshooting"?
> 
> I just felt it would be easier for a content person to write the actual name
of
> the category, than to know what the simplified category name was.

Acknowledged.

[ire, 2017-10-12 12:43:24]

Updated and ready for review

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
File includes/adblockplus/remove-a-website-from-the-whitelist.md (right):

https://codereview.adblockplus.org/29550574/diff/29556555/includes/adblockplu...
includes/adblockplus/remove-a-website-from-the-whitelist.md:96: <? include
no-content-for-platform-message ?>
On 2017/10/09 22:41:31, juliandoucette wrote:
> On 2017/10/09 20:20:47, ire wrote:
> > Could you please expand on this. What sort of <template> are you referring
to?
> 
> Show all available instructions by default; // **no-js**
> 
> if (the user's platform can be detected) { // **js-only**
>     hide instructions that do not match the user's platform;
>     if (there are no instructions for the users platform) {
>         show the error message;
>     }
> }
> 
> Place the error message inside of a <script type="text/template"> or
<template>
> [so that it isn't shown, so that it is not mistaken for content, so that
> JavaScript doesn't have to be aware of it's translation].

Okay got it.

The support for <template> does not seem to be adequate
(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template)

I wasn't able to find much information on the support for <script
type="text/template"> besides this thread on reddit
(https://www.reddit.com/r/webdev/comments/3sek2i/question_template_vs_script_t...).
But since we are just using it to hold some text, I chose to use that route for
this purpose. 

For now, I just created the template and placed it in the article.tmpl . In a
separate issue, I will handle displaying the text in the appropriate place.

https://codereview.adblockplus.org/29550574/diff/29574617/pages/adblockplus/a...
File pages/adblockplus/add-a-filter-list.md (right):

https://codereview.adblockplus.org/29550574/diff/29574617/pages/adblockplus/a...
pages/adblockplus/add-a-filter-list.md:13: <section class="platform-ios"
markdown="1">
I was thinking that, when the user selects a particular browser (or it's
auto-detected), I would check if there is text content within the appropriate
<section>, then if not, replace the content with the template. This may be
easier for new content to be added.

But I wonder if it makes sense to have these <section>s here if they are going
to be empty. I could just as easily check for the existence of a <section> with
the relevant class, then create one with the template content if it doesn't
exist. 

Thoughts?

[juliandoucette, 2017-10-12 13:00:39]

https://codereview.adblockplus.org/29550574/diff/29574617/pages/adblockplus/a...
File pages/adblockplus/add-a-filter-list.md (right):

https://codereview.adblockplus.org/29550574/diff/29574617/pages/adblockplus/a...
pages/adblockplus/add-a-filter-list.md:13: <section class="platform-ios"
markdown="1">
On 2017/10/12 12:43:23, ire wrote:
> Thoughts?

I meant to imply that you don't need empty sections because you can just check
for the absence of a section - in my last comment.

[ire, 2017-10-13 09:28:14]

https://codereview.adblockplus.org/29550574/diff/29574617/pages/adblockplus/a...
File pages/adblockplus/add-a-filter-list.md (right):

https://codereview.adblockplus.org/29550574/diff/29574617/pages/adblockplus/a...
pages/adblockplus/add-a-filter-list.md:13: <section class="platform-ios"
markdown="1">
On 2017/10/12 13:00:38, juliandoucette wrote:
> On 2017/10/12 12:43:23, ire wrote:
> > Thoughts?
> 
> I meant to imply that you don't need empty sections because you can just check
> for the absence of a section - in my last comment.

Done.

[juliandoucette, 2017-10-13 11:09:33]

LGTM