Issue 29329984: Issue 3257 - Create uninstallation page in adblockplus.org

saroyanm

Hey guys can you please have a look when you have time, would like to ...

Nov. 11, 2015, 6 p.m. (2015-11-11 18:00:03 UTC) #1

Thomas Greiner

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp.tmpl#newcode21 pages/uninstall-abp.tmpl:21: window.location.search.substr(1).split("&").forEach(function(param) According to the ticket description the page is ...

Nov. 11, 2015, 7:08 p.m. (2015-11-11 19:08:29 UTC) #2

saroyanm

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp.tmpl#newcode21 pages/uninstall-abp.tmpl:21: window.location.search.substr(1).split("&").forEach(function(param) On 2015/11/11 19:08:29, Thomas Greiner wrote: > According ...

Nov. 12, 2015, 3:50 p.m. (2015-11-12 15:50:25 UTC) #3

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:21:
window.location.search.substr(1).split("&").forEach(function(param)
On 2015/11/11 19:08:29, Thomas Greiner wrote:
> According to the ticket description the page is supposed pass the parameters
> that are listed there. You're not checking for those, however, so I'm
wondering
> whether that's intentional or not.

Yes you right, I should have left a note regarding that in the reaview, It's
actually intentional, In this case we won't need to update the parameters each
time we decide to pass new parameters from ABP.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:29: if (paramName)
On 2015/11/11 19:08:28, Thomas Greiner wrote:
> What do you want to check for? If there's no name then there's no need to add
an
> input field and also each of the parameters should have a value.
> 
> If you still want to check for that I'd suggest doing that at the very
beginning
> of the loop:
> 
> if (!/.=./.test(param))
>   return;

Fare enough. Done.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:34:
document.getElementById("paramsWrapper").appendChild(input);
On 2015/11/11 19:08:28, Thomas Greiner wrote:
> Since those input fields are hidden anyway, it doesn't matter where in the
form
> you add them so I'd suggest instead of introducing a placeholder element,
> appending it directly to the `<form>` element.

Done.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:46:
form.getElementsByTagName("button")[0].addEventListener("click",
On 2015/11/11 19:08:28, Thomas Greiner wrote:
> In this case I'd recommend using `getElementsById` to avoid issues in case
more
> buttons get added to the form (same with "textarea" above).

Done.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:62: <form action="uninstall-abp-submit" method="get">
On 2015/11/11 19:08:28, Thomas Greiner wrote:
> According to the ticket description the form should be submitted via POST.

Ohh wow, thanks.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:74: <input id="reasonOther" type="checkbox"
name="reason" value="0v0" />
On 2015/11/11 19:08:28, Thomas Greiner wrote:
> Detail: This ID value is not corresponding to our coding style (see
>
https://google.github.io/styleguide/htmlcssguide.xml?showone=ID_and_Class_Nam...).

Ahh right! Done.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:81: <button
type="submit">{{"Submit"|translate("submit", "Submition button text")}}</button>
On 2015/11/11 19:08:28, Thomas Greiner wrote:
> Typo: Replace "Submition button text" with "Submit button text"

Done.

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:81: <button
type="submit">{{"Submit"|translate("submit", "Submition button text")}}</button>
On 2015/11/11 19:08:29, Thomas Greiner wrote:
> Note that this form will work even for visitors that have JavaScript disabled.
> The server would then get all of the answers from the visitor but not the
> parameters that the page is given by the extension.
> 
> So either we
> (a) live with that as is (needs to be checked back with data analysts),
> (b) add a hidden form field that tells us that JavaScript was disabled (again,
> needs to be checked back with data analysts),
> (c) disallow non-JavaScript visitors to submit the form or
> (d) create the form fields from the GET parameters dynamically on the server
> side.

I think if we can disable form submission for people without JS enabled we will
be saved from most of bots, anyway the only way for people to reach the form is
by direct link or from extension, the later one should also have JS enabled,
unless there is no option enabling JS only for extensions.

Apart from this question would be great to have @kzar feedback under the review
as well about generating the hidden input content dynamically on server side,
initially we decided to have it in JS, because of complication, but I'll be
still fan of generating it on serverside (that's something we will probably will
need in future as well).

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:82: <span class="error-label">{{"Please select at least
one of the options above"|translate("error-msg", "Error msg, is being shown
after submission if no item selected")}}</span>
On 2015/11/11 19:08:29, Thomas Greiner wrote:
> Detail: Shortening "message" to "msg" here makes the description easier to
> misunderstand for translators and it doesn't seem like we need to care about
> each character at this point.

Done.

https://codereview.adblockplus.org/29329984/diff/29329985/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29329985/static/css/simple.c...
static/css/simple.css:44: a, a:link, a:visited
On 2015/11/11 19:08:29, Thomas Greiner wrote:
> According to our coding style each selector should be on its own line (see
>
https://google.github.io/styleguide/htmlcssguide.xml?showone=Selector_and_Dec...).

Done.

https://codereview.adblockplus.org/29329984/diff/29329985/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29329985/templates/simple.tm...
templates/simple.tmpl:28: <img id="logo" src="/img/adblockplus_new_100.png"
alt="{{"Adblock Plus logo"|translate("abp-logo", "Alternative text for Adblock
Plus logo image")}}" />
On 2015/11/11 19:08:29, Thomas Greiner wrote:
> Detail: I'd strongly advise against using relative words such as "new" in file
> names since whatever is new today may not be new anymore the next time we
update
> the logo.
> 
> Therefore using "/img/adblockplus_100.png" should be sufficient. Also I
noticed
> that we have a "adblockplus_128.png" image in that repository (see
> https://adblockplus.org/img/adblockplus_128.png) so I wonder whether we could
> simply reuse or even update that one.

Aligned with designer before implementing, seems like they are not sure about
final look of the logo in adblockplus.org and would prefer to tackle that topic
later.
While the logos are different it make sense to somehow separate them, what about
"adblockplus_clean_100.png" or "adblockplus_smooth_100.png" ?

saroyanm

https://codereview.adblockplus.org/29329984/diff/29329985/templates/simple.tmpl File templates/simple.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29329985/templates/simple.tmpl#newcode28 templates/simple.tmpl:28: <img id="logo" src="/img/adblockplus_new_100.png" alt="{{"Adblock Plus logo"|translate("abp-logo", "Alternative text for ...

Nov. 12, 2015, 4:11 p.m. (2015-11-12 16:11:16 UTC) #4

kzar

https://codereview.adblockplus.org/29329984/diff/29330137/filters/shuffle.py File filters/shuffle.py (right): https://codereview.adblockplus.org/29329984/diff/29330137/filters/shuffle.py#newcode18 filters/shuffle.py:18: def shuffle(seq): As mentioned in my other comment, shuffling ...

Nov. 16, 2015, 10:55 a.m. (2015-11-16 10:55:19 UTC) #5

saroyanm

https://codereview.adblockplus.org/29329984/diff/29330137/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330137/pages/uninstall-abp.tmpl#newcode4 pages/uninstall-abp.tmpl:4: {% set reasons = [ On 2015/11/16 10:55:19, kzar ...

Nov. 17, 2015, 3:27 p.m. (2015-11-17 15:27:50 UTC) #6

kzar

https://codereview.adblockplus.org/29329984/diff/29330137/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330137/pages/uninstall-abp.tmpl#newcode4 pages/uninstall-abp.tmpl:4: {% set reasons = [ On 2015/11/17 15:27:49, saroyanm ...

Nov. 18, 2015, 5:14 p.m. (2015-11-18 17:14:45 UTC) #7

saroyanm

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl#newcode72 pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %} On ...

Nov. 18, 2015, 6 p.m. (2015-11-18 18:00:19 UTC) #8

kzar

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl#newcode72 pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %} On ...

Nov. 19, 2015, 11:24 a.m. (2015-11-19 11:24:55 UTC) #9

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %}
On 2015/11/18 18:00:18, saroyanm wrote:
> On 2015/11/18 17:14:45, kzar wrote:
> > I think it would make more sense to add a to_json Jinja2 filter, and then
use
> > that to dump the reasons to a JavaScript array. You could then generate the
> > shuffled elements directly in JavaScript, instead of putting them here and
> later
> > removing them + appending them again in JavaScript.
> Fare enough, I agree that this is not a good solution I have now.
> 
> > In fact, you could simply declare the list of reasons as a JSON array in the
> > first place near the top of the file. Then you would no longer need the
> to_json
> > filter and this file wouldn't even need to be a template at all.
> Not sure if in that case I can get translations for reasons, also the
> implementation should be supported by translation script to be able to upload
> strings to crowdin, this also applies to the first solution you suggested.

Make this page Markdown, you don't need it to be a Jinja2 template. Do something
like this:

title=Adblock Plus has been uninstalled
template=simple

<head>
  <script type="text/javascript">
    var reasons = [
      ["1v0", "{{reason-not-installed I didn't install Adblock Plus.}}"],
      ["2v0", "{{reason-slowing-down Adblock Plus slowed down my browser.}}"],
      ["3v0", "{{reason-acceptable-ads I don't like the Acceptable Ads
program.}}"],
      ["4v0", "{{reason-see-ads Adblock Plus didn't block all ads.}}"],
      ["5v0", "{{reason-better-adblocker I found better ad blocking
software.}}"],
      ["6v0", "{{reason-break-websites Adblock Plus breaks websites that I
visit.}}"]
    ];

    ...
  </script>
</head>

<section class="highlighted">
  <h1>{{reasons-header Please select the reason(s) why you uninstalled Adblock
Plus:</h1>

  <form id="reasons-form" action="uninstall-abp-submit" method="post">
    <ul id="reasons"></ul>

    ...
  </form>
  ...
</section>

Notes:
 - Yes quotes in translated strings are escaped, I checked.
 - My example uses HTML for markup, you should obviously only use it where
required and use Markdown if possible.
 - Please create a function called shuffle to do the shuffling and use that to
shuffle the array, instead of mixing that logic with the rest of the
initialisation logic.

kzar

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl#newcode72 pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %} On ...

Nov. 19, 2015, 11:28 a.m. (2015-11-19 11:28:56 UTC) #10

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %}
On 2015/11/19 11:24:54, kzar wrote:
> On 2015/11/18 18:00:18, saroyanm wrote:
> > On 2015/11/18 17:14:45, kzar wrote:
> > > I think it would make more sense to add a to_json Jinja2 filter, and then
> use
> > > that to dump the reasons to a JavaScript array. You could then generate
the
> > > shuffled elements directly in JavaScript, instead of putting them here and
> > later
> > > removing them + appending them again in JavaScript.
> > Fare enough, I agree that this is not a good solution I have now.
> > 
> > > In fact, you could simply declare the list of reasons as a JSON array in
the
> > > first place near the top of the file. Then you would no longer need the
> > to_json
> > > filter and this file wouldn't even need to be a template at all.
> > Not sure if in that case I can get translations for reasons, also the
> > implementation should be supported by translation script to be able to
upload
> > strings to crowdin, this also applies to the first solution you suggested.
> 
> Make this page Markdown, you don't need it to be a Jinja2 template. Do
something
> like this:
> 
> title=Adblock Plus has been uninstalled
> template=simple
> 
> <head>
>   <script type="text/javascript">
>     var reasons = [
>       ["1v0", "{{reason-not-installed I didn't install Adblock Plus.}}"],
>       ["2v0", "{{reason-slowing-down Adblock Plus slowed down my browser.}}"],
>       ["3v0", "{{reason-acceptable-ads I don't like the Acceptable Ads
> program.}}"],
>       ["4v0", "{{reason-see-ads Adblock Plus didn't block all ads.}}"],
>       ["5v0", "{{reason-better-adblocker I found better ad blocking
> software.}}"],
>       ["6v0", "{{reason-break-websites Adblock Plus breaks websites that I
> visit.}}"]
>     ];
> 
>     ...
>   </script>
> </head>
> 
> <section class="highlighted">
>   <h1>{{reasons-header Please select the reason(s) why you uninstalled Adblock
> Plus:</h1>
> 
>   <form id="reasons-form" action="uninstall-abp-submit" method="post">
>     <ul id="reasons"></ul>
> 
>     ...
>   </form>
>   ...
> </section>
> 
> Notes:
>  - Yes quotes in translated strings are escaped, I checked.
>  - My example uses HTML for markup, you should obviously only use it where
> required and use Markdown if possible.
>  - Please create a function called shuffle to do the shuffling and use that to
> shuffle the array, instead of mixing that logic with the rest of the
> initialisation logic.

Also why not keep the string ID the same as the ID the data guys will use?
Instead of having "1v0" and "reason-not-installed" why not call both
"reason-1v0-not-installed" or something? It seems like extra work for them to
have to keep looking up string IDs like "1v0" -> "reason-not-installed" ->
"blah".

saroyanm

New patch uploaded, I've also added the noscript implementation, while the options will be available ...

Nov. 19, 2015, 5 p.m. (2015-11-19 17:00:18 UTC) #11

New patch uploaded, I've also added the noscript implementation, while the
options will be available to the users who has JS enabled we need to prevent
users form submission in case they do not have JS enabled.

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %}
On 2015/11/19 11:24:54, kzar wrote:
> On 2015/11/18 18:00:18, saroyanm wrote:
> > On 2015/11/18 17:14:45, kzar wrote:
> > > I think it would make more sense to add a to_json Jinja2 filter, and then
> use
> > > that to dump the reasons to a JavaScript array. You could then generate
the
> > > shuffled elements directly in JavaScript, instead of putting them here and
> > later
> > > removing them + appending them again in JavaScript.
> > Fare enough, I agree that this is not a good solution I have now.
> > 
> > > In fact, you could simply declare the list of reasons as a JSON array in
the
> > > first place near the top of the file. Then you would no longer need the
> > to_json
> > > filter and this file wouldn't even need to be a template at all.
> > Not sure if in that case I can get translations for reasons, also the
> > implementation should be supported by translation script to be able to
upload
> > strings to crowdin, this also applies to the first solution you suggested.
> 
> Make this page Markdown, you don't need it to be a Jinja2 template. Do
something
> like this:
> 
> title=Adblock Plus has been uninstalled
> template=simple
> 
> <head>
>   <script type="text/javascript">
>     var reasons = [
>       ["1v0", "{{reason-not-installed I didn't install Adblock Plus.}}"],
>       ["2v0", "{{reason-slowing-down Adblock Plus slowed down my browser.}}"],
>       ["3v0", "{{reason-acceptable-ads I don't like the Acceptable Ads
> program.}}"],
>       ["4v0", "{{reason-see-ads Adblock Plus didn't block all ads.}}"],
>       ["5v0", "{{reason-better-adblocker I found better ad blocking
> software.}}"],
>       ["6v0", "{{reason-break-websites Adblock Plus breaks websites that I
> visit.}}"]
>     ];
>
>     ...
>   </script>
> </head>
> 
> <section class="highlighted">
>   <h1>{{reasons-header Please select the reason(s) why you uninstalled Adblock
> Plus:</h1>
> 
>   <form id="reasons-form" action="uninstall-abp-submit" method="post">
>     <ul id="reasons"></ul>
> 
>     ...
>   </form>
>   ...
> </section>
> 
> Notes:
>  - Yes quotes in translated strings are escaped, I checked.
>  - My example uses HTML for markup, you should obviously only use it where
> required and use Markdown if possible.
>  - Please create a function called shuffle to do the shuffling and use that to
> shuffle the array, instead of mixing that logic with the rest of the
> initialisation logic.

Ohh wow, didn't considered that case, for some reason I was thinking that we
can't have {{}} syntax in JavaScript, now I see your point, sorry for confusion
indeed better solution. 

Please note that technically I'm not shuffling the array, but looping through
array randomly, not sure if we need function for that, I've added comment to
separate the implementation if it will work for you, otherwise I can have
separate function for that if you insist.

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %}
On 2015/11/19 11:28:56, kzar wrote:
> On 2015/11/19 11:24:54, kzar wrote:
> > On 2015/11/18 18:00:18, saroyanm wrote:
> > > On 2015/11/18 17:14:45, kzar wrote:
> > > > I think it would make more sense to add a to_json Jinja2 filter, and
then
> > use
> > > > that to dump the reasons to a JavaScript array. You could then generate
> the
> > > > shuffled elements directly in JavaScript, instead of putting them here
and
> > > later
> > > > removing them + appending them again in JavaScript.
> > > Fare enough, I agree that this is not a good solution I have now.
> > > 
> > > > In fact, you could simply declare the list of reasons as a JSON array in
> the
> > > > first place near the top of the file. Then you would no longer need the
> > > to_json
> > > > filter and this file wouldn't even need to be a template at all.
> > > Not sure if in that case I can get translations for reasons, also the
> > > implementation should be supported by translation script to be able to
> upload
> > > strings to crowdin, this also applies to the first solution you suggested.
> > 
> > Make this page Markdown, you don't need it to be a Jinja2 template. Do
> something
> > like this:
> > 
> > title=Adblock Plus has been uninstalled
> > template=simple
> > 
> > <head>
> >   <script type="text/javascript">
> >     var reasons = [
> >       ["1v0", "{{reason-not-installed I didn't install Adblock Plus.}}"],
> >       ["2v0", "{{reason-slowing-down Adblock Plus slowed down my
browser.}}"],
> >       ["3v0", "{{reason-acceptable-ads I don't like the Acceptable Ads
> > program.}}"],
> >       ["4v0", "{{reason-see-ads Adblock Plus didn't block all ads.}}"],
> >       ["5v0", "{{reason-better-adblocker I found better ad blocking
> > software.}}"],
> >       ["6v0", "{{reason-break-websites Adblock Plus breaks websites that I
> > visit.}}"]
> >     ];
> > 
> >     ...
> >   </script>
> > </head>
> > 
> > <section class="highlighted">
> >   <h1>{{reasons-header Please select the reason(s) why you uninstalled
Adblock
> > Plus:</h1>
> > 
> >   <form id="reasons-form" action="uninstall-abp-submit" method="post">
> >     <ul id="reasons"></ul>
> > 
> >     ...
> >   </form>
> >   ...
> > </section>
> > 
> > Notes:
> >  - Yes quotes in translated strings are escaped, I checked.
> >  - My example uses HTML for markup, you should obviously only use it where
> > required and use Markdown if possible.
> >  - Please create a function called shuffle to do the shuffling and use that
to
> > shuffle the array, instead of mixing that logic with the rest of the
> > initialisation logic.
> 
> Also why not keep the string ID the same as the ID the data guys will use?
> Instead of having "1v0" and "reason-not-installed" why not call both
> "reason-1v0-not-installed" or something? It seems like extra work for them to
> have to keep looking up string IDs like "1v0" -> "reason-not-installed" ->
> "blah".

It was requirement by data annalists to have request like:
?reason=1v0&reason=4v0&reason=2v0&other=some%20reason

I can use  1v0, 2v0 and etc.. as Message ID, but not sure if it's okey to have
meaningless Ids as messageId. Also it will require to keep in sync Message Ids
and reason Ids together.

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tm...
templates/simple.tmpl:32: <section class="highlighted">
I moved the section here, not sure if it's a good idea would be nice to hear
second opinion regarding that, the problem is that in case we will have several
section layouts (eg. as in the firstRun page) we will need to change this
implementation.

kzar

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp.tmpl#newcode72 pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %} On ...

Nov. 20, 2015, 1:06 p.m. (2015-11-20 13:06:15 UTC) #12

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
pages/uninstall-abp.tmpl:72: {%- for reasonId, stringId, value in reasons %}
On 2015/11/19 17:00:17, saroyanm wrote:
> On 2015/11/19 11:28:56, kzar wrote:
> > On 2015/11/19 11:24:54, kzar wrote:
> > > On 2015/11/18 18:00:18, saroyanm wrote:
> > > > On 2015/11/18 17:14:45, kzar wrote:
> > > > > I think it would make more sense to add a to_json Jinja2 filter, and
> then
> > > use
> > > > > that to dump the reasons to a JavaScript array. You could then
generate
> > the
> > > > > shuffled elements directly in JavaScript, instead of putting them here
> and
> > > > later
> > > > > removing them + appending them again in JavaScript.
> > > > Fare enough, I agree that this is not a good solution I have now.
> > > > 
> > > > > In fact, you could simply declare the list of reasons as a JSON array
in
> > the
> > > > > first place near the top of the file. Then you would no longer need
the
> > > > to_json
> > > > > filter and this file wouldn't even need to be a template at all.
> > > > Not sure if in that case I can get translations for reasons, also the
> > > > implementation should be supported by translation script to be able to
> > upload
> > > > strings to crowdin, this also applies to the first solution you
suggested.
> > > 
> > > Make this page Markdown, you don't need it to be a Jinja2 template. Do
> > something
> > > like this:
> > > 
> > > title=Adblock Plus has been uninstalled
> > > template=simple
> > > 
> > > <head>
> > >   <script type="text/javascript">
> > >     var reasons = [
> > >       ["1v0", "{{reason-not-installed I didn't install Adblock Plus.}}"],
> > >       ["2v0", "{{reason-slowing-down Adblock Plus slowed down my
> browser.}}"],
> > >       ["3v0", "{{reason-acceptable-ads I don't like the Acceptable Ads
> > > program.}}"],
> > >       ["4v0", "{{reason-see-ads Adblock Plus didn't block all ads.}}"],
> > >       ["5v0", "{{reason-better-adblocker I found better ad blocking
> > > software.}}"],
> > >       ["6v0", "{{reason-break-websites Adblock Plus breaks websites that I
> > > visit.}}"]
> > >     ];
> > > 
> > >     ...
> > >   </script>
> > > </head>
> > > 
> > > <section class="highlighted">
> > >   <h1>{{reasons-header Please select the reason(s) why you uninstalled
> Adblock
> > > Plus:</h1>
> > > 
> > >   <form id="reasons-form" action="uninstall-abp-submit" method="post">
> > >     <ul id="reasons"></ul>
> > > 
> > >     ...
> > >   </form>
> > >   ...
> > > </section>
> > > 
> > > Notes:
> > >  - Yes quotes in translated strings are escaped, I checked.
> > >  - My example uses HTML for markup, you should obviously only use it where
> > > required and use Markdown if possible.
> > >  - Please create a function called shuffle to do the shuffling and use
that
> to
> > > shuffle the array, instead of mixing that logic with the rest of the
> > > initialisation logic.
> > 
> > Also why not keep the string ID the same as the ID the data guys will use?
> > Instead of having "1v0" and "reason-not-installed" why not call both
> > "reason-1v0-not-installed" or something? It seems like extra work for them
to
> > have to keep looking up string IDs like "1v0" -> "reason-not-installed" ->
> > "blah".
> 
> It was requirement by data annalists to have request like:
> ?reason=1v0&reason=4v0&reason=2v0&other=some%20reason
> 
> I can use  1v0, 2v0 and etc.. as Message ID, but not sure if it's okey to have
> meaningless Ids as messageId. Also it will require to keep in sync Message Ids
> and reason Ids together.

OK fair enough, thanks the latest version of this page is looking a lot more
sensible now.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:22: // create hidden input for GET parameters
Nit: Capitalize "create" to match the comment below

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:37: while (reasons.length > 0)
Would it make more sense to use a for loop here? Something like this:

for (var i = reasons.length; i > 0; i -= 1)
{
  var reason = reasons.splice(Math.floor(Math.random() * i), 1)[0];

  ...
}

(I've tested this and the results seem about right, n! groups with a pretty even
distribution.)

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:96: <span>{{noscript[No script notification] Form can not
be submited with disabled JavaScript}}</span>
If the user doesn't have JavaScript then we can't get any of the information
from them right? Well if so I don't think we should bother display an error
message about that, it will probably confuse them and frankly they won't care.
They wanted to uninstall the extension, not share information.

I vote we just display a successful message stating that the extension has been
uninstalled if JavaScript is disabled.

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tm...
templates/simple.tmpl:32: <section class="highlighted">
On 2015/11/19 17:00:17, saroyanm wrote:
> I moved the section here, not sure if it's a good idea would be nice to hear
> second opinion regarding that, the problem is that in case we will have
several
> section layouts (eg. as in the firstRun page) we will need to change this
> implementation.

Well IMHO you should keep it out of the template in that case, if a template
isn't reusable it's kinda pointless.

saroyanm

@Dave new patches uploaded, can you please have a look. Please note now there are ...

Nov. 20, 2015, 6:33 p.m. (2015-11-20 18:33:55 UTC) #13

@Dave new patches uploaded, can you please have a look.
Please note now there are 3 different reviews. 
Patchset #8: I've just addressed your comments that are irrelevant to "section"
tag removal from template. 
Patchset #9: I've just renamed the markdown files back to template.
Patchset #10: Implemented required changes to move "section" tag from template.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:22: // create hidden input for GET parameters
On 2015/11/20 13:06:13, kzar wrote:
> Nit: Capitalize "create" to match the comment below

Done.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:37: while (reasons.length > 0)
On 2015/11/20 13:06:13, kzar wrote:
> Would it make more sense to use a for loop here? Something like this:
> 
> for (var i = reasons.length; i > 0; i -= 1)
> {
>   var reason = reasons.splice(Math.floor(Math.random() * i), 1)[0];
> 
>   ...
> }
> 
> (I've tested this and the results seem about right, n! groups with a pretty
even
> distribution.)

Not sure why it makes more sense. From my point of view the while is more clear
in this case, while there is no need to assign variable and decrement it in for
loop, while in this case I'm just using existing size of array.
IMO the while implementation here is more simple, but if you insist on for
implementation or can convince me why the for implementation is better here will
be happy to change the implementation.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:96: <span>{{noscript[No script notification] Form can not
be submited with disabled JavaScript}}</span>
On 2015/11/20 13:06:14, kzar wrote:
> If the user doesn't have JavaScript then we can't get any of the information
> from them right? Well if so I don't think we should bother display an error
> message about that, it will probably confuse them and frankly they won't care.
> They wanted to uninstall the extension, not share information.
>
> I vote we just display a successful message stating that the extension has
been
> uninstalled if JavaScript is disabled.

Can see any good way to do that.
I've just removed the message completely, assuming that users who should end up
on this page probably need to have javascript enabled, while as you mentioned we
expect them being redirected from extension, which do require JS, but on other
hand it can save us fake bots submission, so I've removed the message but left
the submission only for JS enabled users.

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tm...
templates/simple.tmpl:32: <section class="highlighted">
On 2015/11/20 13:06:14, kzar wrote:
> On 2015/11/19 17:00:17, saroyanm wrote:
> > I moved the section here, not sure if it's a good idea would be nice to hear
> > second opinion regarding that, the problem is that in case we will have
> several
> > section layouts (eg. as in the firstRun page) we will need to change this
> > implementation.
> 
> Well IMHO you should keep it out of the template in that case, if a template
> isn't reusable it's kinda pointless.

I agree with you, the only problem is that I can't write markdown in block level
html element tag and only for that reason need to have separate file in includes
directory. So I'll end up with the additional 2 files, that why I was having a
doubt, but seams like we do not have a choice.

saroyanm

https://codereview.adblockplus.org/29329984/diff/29330605/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330605/pages/uninstall-abp.tmpl#newcode9 pages/uninstall-abp.tmpl:9: var reasons = [ I've just noticed that the ...

Nov. 20, 2015, 6:36 p.m. (2015-11-20 18:36:18 UTC) #14

kzar

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md#newcode37 pages/uninstall-abp.md:37: while (reasons.length > 0) On 2015/11/20 18:33:53, saroyanm wrote: ...

Nov. 21, 2015, 7:01 p.m. (2015-11-21 19:01:26 UTC) #15

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:37: while (reasons.length > 0)
On 2015/11/20 18:33:53, saroyanm wrote:
> On 2015/11/20 13:06:13, kzar wrote:
> > Would it make more sense to use a for loop here? Something like this:
> > 
> > for (var i = reasons.length; i > 0; i -= 1)
> > {
> >   var reason = reasons.splice(Math.floor(Math.random() * i), 1)[0];
> > 
> >   ...
> > }
> > 
> > (I've tested this and the results seem about right, n! groups with a pretty
> even
> > distribution.)
> 
> Not sure why it makes more sense. From my point of view the while is more
clear
> in this case, while there is no need to assign variable and decrement it in
for
> loop, while in this case I'm just using existing size of array.
> IMO the while implementation here is more simple, but if you insist on for
> implementation or can convince me why the for implementation is better here
will
> be happy to change the implementation.

Well in your current implementation you're (re)declaring randomIndex for each
iteration, where as using my suggestion you would be declaring i once and
decrementing it once per iteration. IMHO my suggestion makes more sense but I
have no interest arguing about it.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:96: <span>{{noscript[No script notification] Form can not
be submited with disabled JavaScript}}</span>
On 2015/11/20 18:33:53, saroyanm wrote:
> On 2015/11/20 13:06:14, kzar wrote:
> > If the user doesn't have JavaScript then we can't get any of the information
> > from them right? Well if so I don't think we should bother display an error
> > message about that, it will probably confuse them and frankly they won't
care.
> > They wanted to uninstall the extension, not share information.
> >
> > I vote we just display a successful message stating that the extension has
> been
> > uninstalled if JavaScript is disabled.
> 
> Can see any good way to do that.
> I've just removed the message completely, assuming that users who should end
up
> on this page probably need to have javascript enabled, while as you mentioned
we
> expect them being redirected from extension, which do require JS, but on other
> hand it can save us fake bots submission, so I've removed the message but left
> the submission only for JS enabled users.

Perhaps we should discuss what should happen for non JS users on the issue?

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tm...
templates/simple.tmpl:32: <section class="highlighted">
On 2015/11/20 18:33:54, saroyanm wrote:
> On 2015/11/20 13:06:14, kzar wrote:
> > On 2015/11/19 17:00:17, saroyanm wrote:
> > > I moved the section here, not sure if it's a good idea would be nice to
hear
> > > second opinion regarding that, the problem is that in case we will have
> > several
> > > section layouts (eg. as in the firstRun page) we will need to change this
> > > implementation.
> > 
> > Well IMHO you should keep it out of the template in that case, if a template
> > isn't reusable it's kinda pointless.
> 
> I agree with you, the only problem is that I can't write markdown in block
level
> html element tag and only for that reason need to have separate file in
includes
> directory. So I'll end up with the additional 2 files, that why I was having a
> doubt, but seams like we do not have a choice.

I'm not sure that I understand to be honest, but I really dislike how
uninstall-abp.tmpl and uninstall-abp-submit.tmpl files have been added. Is there
truely no way to avoid that? Not only are we not actually using any Jinja2
functionality in those pages, but worse there are now two files for each page.

https://codereview.adblockplus.org/29329984/diff/29330605/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330605/pages/uninstall-abp...
pages/uninstall-abp.tmpl:9: var reasons = [
On 2015/11/20 18:36:17, saroyanm wrote:
> I've just noticed that the syntax highlighting being screwed up bit in the
> editors, bit annoying.

Perhaps mixing " and 's would make it easier for your editor to highlight?

["1v0", "{{'I didn\'t install Adblock Plus...

saroyanm

New patch uploaded. https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md#newcode37 pages/uninstall-abp.md:37: while (reasons.length > 0) On 2015/11/21 ...

Nov. 23, 2015, 10:50 a.m. (2015-11-23 10:50:32 UTC) #16

New patch uploaded.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:37: while (reasons.length > 0)
On 2015/11/21 19:01:24, kzar wrote:
> On 2015/11/20 18:33:53, saroyanm wrote:
> > On 2015/11/20 13:06:13, kzar wrote:
> > > Would it make more sense to use a for loop here? Something like this:
> > > 
> > > for (var i = reasons.length; i > 0; i -= 1)
> > > {
> > >   var reason = reasons.splice(Math.floor(Math.random() * i), 1)[0];
> > > 
> > >   ...
> > > }
> > > 
> > > (I've tested this and the results seem about right, n! groups with a
pretty
> > even
> > > distribution.)
> > 
> > Not sure why it makes more sense. From my point of view the while is more
> clear
> > in this case, while there is no need to assign variable and decrement it in
> for
> > loop, while in this case I'm just using existing size of array.
> > IMO the while implementation here is more simple, but if you insist on for
> > implementation or can convince me why the for implementation is better here
> will
> > be happy to change the implementation.
> 
> Well in your current implementation you're (re)declaring randomIndex for each
> iteration, where as using my suggestion you would be declaring i once and
> decrementing it once per iteration. IMHO my suggestion makes more sense but I
> have no interest arguing about it.

Well, with your implementation we also generating random index on each
iteration, the only difference that I'm assigning it to the variable for
readability. 
But as you say, changed to "for" loop. Not a big thing to argue about.

https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
pages/uninstall-abp.md:96: <span>{{noscript[No script notification] Form can not
be submited with disabled JavaScript}}</span>
On 2015/11/21 19:01:24, kzar wrote:
> On 2015/11/20 18:33:53, saroyanm wrote:
> > On 2015/11/20 13:06:14, kzar wrote:
> > > If the user doesn't have JavaScript then we can't get any of the
information
> > > from them right? Well if so I don't think we should bother display an
error
> > > message about that, it will probably confuse them and frankly they won't
> care.
> > > They wanted to uninstall the extension, not share information.
> > >
> > > I vote we just display a successful message stating that the extension has
> > been
> > > uninstalled if JavaScript is disabled.
> > 
> > Can see any good way to do that.
> > I've just removed the message completely, assuming that users who should end
> up
> > on this page probably need to have javascript enabled, while as you
mentioned
> we
> > expect them being redirected from extension, which do require JS, but on
other
> > hand it can save us fake bots submission, so I've removed the message but
left
> > the submission only for JS enabled users.
> 
> Perhaps we should discuss what should happen for non JS users on the issue?

I can see a reason of discussing this question in ticket if there is a scenario
when user will end up on this page after uninstallation with disabled JS, I can
only imagine that scenario by direct link, which looks to be unlikely.

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tm...
templates/simple.tmpl:32: <section class="highlighted">
On 2015/11/21 19:01:24, kzar wrote:
> On 2015/11/20 18:33:54, saroyanm wrote:
> > On 2015/11/20 13:06:14, kzar wrote:
> > > On 2015/11/19 17:00:17, saroyanm wrote:
> > > > I moved the section here, not sure if it's a good idea would be nice to
> hear
> > > > second opinion regarding that, the problem is that in case we will have
> > > several
> > > > section layouts (eg. as in the firstRun page) we will need to change
this
> > > > implementation.
> > > 
> > > Well IMHO you should keep it out of the template in that case, if a
template
> > > isn't reusable it's kinda pointless.
> > 
> > I agree with you, the only problem is that I can't write markdown in block
> level
> > html element tag and only for that reason need to have separate file in
> includes
> > directory. So I'll end up with the additional 2 files, that why I was having
a
> > doubt, but seams like we do not have a choice.
> 
> I'm not sure that I understand to be honest, but I really dislike how
> uninstall-abp.tmpl and uninstall-abp-submit.tmpl files have been added. Is
there
> truely no way to avoid that? Not only are we not actually using any Jinja2
> functionality in those pages, but worse there are now two files for each page.

"Unlike block-level HTML tags, Markdown syntax is processed within span-level
tags."
reference -> http://daringfireball.net/projects/markdown/syntax#html 
while "section" is block level element I can't use have markdwon implementation
as ex. below:
<section>
#markdown
</section>

But I can use ex.: "span" element for that purpose, which technically feasible,
but wrong by design:
<span>
#markdown
</span>

So I can see only 2 solutions here:
1. Move "section" element to simple.tmpl and find an extension that will allow
us have block level surrounding markdown elements.
2. separate unistall-abp.tmpl and unistall-abp.md like we have done currently

Hope I could describe better the issue, let me know if it still not clear.

https://codereview.adblockplus.org/29329984/diff/29330605/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330605/pages/uninstall-abp...
pages/uninstall-abp.tmpl:9: var reasons = [
On 2015/11/21 19:01:25, kzar wrote:
> On 2015/11/20 18:36:17, saroyanm wrote:
> > I've just noticed that the syntax highlighting being screwed up bit in the
> > editors, bit annoying.
> 
> Perhaps mixing " and 's would make it easier for your editor to highlight?
> 
> ["1v0", "{{'I didn\'t install Adblock Plus...

Yes, yes, but from point of view making it as manageable as possible is not the
best solution, because people will need to double check and be sure that they
are escaping the characters, but I do not have a strong opinion which is better,
so went with your suggestion.

kzar

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl File templates/simple.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl#newcode32 templates/simple.tmpl:32: <section class="highlighted"> On 2015/11/23 10:50:30, saroyanm wrote: > On ...

Nov. 25, 2015, 1:26 p.m. (2015-11-25 13:26:00 UTC) #17

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29330503/templates/simple.tm...
templates/simple.tmpl:32: <section class="highlighted">
On 2015/11/23 10:50:30, saroyanm wrote:
> On 2015/11/21 19:01:24, kzar wrote:
> > On 2015/11/20 18:33:54, saroyanm wrote:
> > > On 2015/11/20 13:06:14, kzar wrote:
> > > > On 2015/11/19 17:00:17, saroyanm wrote:
> > > > > I moved the section here, not sure if it's a good idea would be nice
to
> > hear
> > > > > second opinion regarding that, the problem is that in case we will
have
> > > > several
> > > > > section layouts (eg. as in the firstRun page) we will need to change
> this
> > > > > implementation.
> > > > 
> > > > Well IMHO you should keep it out of the template in that case, if a
> template
> > > > isn't reusable it's kinda pointless.
> > > 
> > > I agree with you, the only problem is that I can't write markdown in block
> > level
> > > html element tag and only for that reason need to have separate file in
> > includes
> > > directory. So I'll end up with the additional 2 files, that why I was
having
> a
> > > doubt, but seams like we do not have a choice.
> > 
> > I'm not sure that I understand to be honest, but I really dislike how
> > uninstall-abp.tmpl and uninstall-abp-submit.tmpl files have been added. Is
> there
> > truely no way to avoid that? Not only are we not actually using any Jinja2
> > functionality in those pages, but worse there are now two files for each
page.
> 
> "Unlike block-level HTML tags, Markdown syntax is processed within span-level
> tags."
> reference -> http://daringfireball.net/projects/markdown/syntax#html 
> while "section" is block level element I can't use have markdwon
implementation
> as ex. below:
> <section>
> #markdown
> </section>
> 
> But I can use ex.: "span" element for that purpose, which technically
feasible,
> but wrong by design:
> <span>
> #markdown
> </span>
> 
> So I can see only 2 solutions here:
> 1. Move "section" element to simple.tmpl and find an extension that will allow
> us have block level surrounding markdown elements.
> 2. separate unistall-abp.tmpl and unistall-abp.md like we have done currently
> 
> Hope I could describe better the issue, let me know if it still not clear.

Thomas is probably better placed to suggest something here but to me it seems
crazy that we now have four files for those two pages, especially as two of them
are Jinja2 templates needlessly.

I mean surely using a span element wrongly is preferable to that? Or just having
one .html file per page instead of a .md and a .tmpl?

kzar

It's looking better and better, noticed a few things though. https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md#newcode10 ...

Nov. 27, 2015, 1:38 p.m. (2015-11-27 13:38:41 UTC) #19

saroyanm

New patch uploaded. https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md#newcode10 pages/uninstall-abp.md:10: ["1v0", "{{reason-not-installed[reason] I didn't install Adblock ...

Nov. 27, 2015, 2:26 p.m. (2015-11-27 14:26:42 UTC) #20

New patch uploaded.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:10: ["1v0", "{{reason-not-installed[reason] I didn't
install Adblock Plus.}}"],
On 2015/11/27 13:38:38, kzar wrote:
> Did you forget to fill out the descriptions for these reason strings?

No, the description is -> reason, but I've updated to "Uninstallation reason",
to make it consistent with the "Last uninstallation reason".

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:31: input.setAttribute("name", paramSplit[0]);
On 2015/11/27 13:38:39, kzar wrote:
> I _think_ it's OK but have you double checked that this is safe from XSS
> attacks? After all paramSplit has come straight from user input and we're now
> inserting it into the DOM.

We should be fine using setAttribute the value should be encoded, unless it's
not script attribute I guess, or href, can't see any issue with name attribute,
should be fine, also tested just in case.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:41: var labelElement = document.createElement("label");
On 2015/11/27 13:38:38, kzar wrote:
> Perhaps we should give the labels "for" attributes linking them to the
> checkboxes? That way clicking on them should activate the checkboxes.

No, in that case I'll need redundant id assigned to checkbox, please note the
checkbox and text are inside label, this still will allow user to change
checkbox state by clicking on the text.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:44: checkboxElement.setAttribute("name", "reason");
On 2015/11/27 13:38:37, kzar wrote:
> Did you mean to give all the reason checkboxes the same name of "reason"?

Yes, exactly, it was intentional.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:56: function()
On 2015/11/27 13:38:39, kzar wrote:
> Nit: Shouldn't the function be indented a couple more spaces?

If be honest it looks bit weird and I had to indent enclosed, so I decided to
assign the element to variable, hope you are okey with that.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:59: textArea.hasAttribute("class") ?
textArea.removeAttribute("class") :
On 2015/11/27 13:38:38, kzar wrote:
> Nit: Indentation here seems weird, maybe it would look better as a normal if
...
> else ... ?

Done.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:72: form.submit();
On 2015/11/27 13:38:37, kzar wrote:
> Nit: I've been told before that with if ... else ... statements the braces
have
> to be used consistently. In other words use braces for both clauses or
neither.

Done.

https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp...
pages/uninstall-abp.md:80: <section markdown="1" class="highlighted">
On 2015/11/27 13:38:39, kzar wrote:
> Nit: Shouldn't the contents of <section> be indented with a couple more
spaces?

According to the Python example no, I've also tried and it's screwed the
content:
https://pythonhosted.org/Markdown/extensions/extra.html#simple-example

https://codereview.adblockplus.org/29329984/diff/29331126/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29331126/templates/simple.tm...
templates/simple.tmpl:19: <html lang="{{locale}}" dir="{{"rtl" if
config.has_option("rtl", locale) else "ltr"}}">
On 2015/11/27 13:38:40, kzar wrote:
> Have you tested that your pages work properly with a RTL language?

Yep, looks fine.

kzar

LGTM https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331126/pages/uninstall-abp.md#newcode41 pages/uninstall-abp.md:41: var labelElement = document.createElement("label"); On 2015/11/27 14:26:39, saroyanm ...

Nov. 27, 2015, 2:30 p.m. (2015-11-27 14:30:43 UTC) #21

Thomas Greiner

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp.tmpl#newcode81 pages/uninstall-abp.tmpl:81: <button type="submit">{{"Submit"|translate("submit", "Submition button text")}}</button> On 2015/11/12 15:50:24, saroyanm ...

Nov. 27, 2015, 7:10 p.m. (2015-11-27 19:10:17 UTC) #22

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29329985/pages/uninstall-abp...
pages/uninstall-abp.tmpl:81: <button
type="submit">{{"Submit"|translate("submit", "Submition button text")}}</button>
On 2015/11/12 15:50:24, saroyanm wrote:
> On 2015/11/11 19:08:29, Thomas Greiner wrote:
> > Note that this form will work even for visitors that have JavaScript
disabled.
> > The server would then get all of the answers from the visitor but not the
> > parameters that the page is given by the extension.
> > 
> > So either we
> > (a) live with that as is (needs to be checked back with data analysts),
> > (b) add a hidden form field that tells us that JavaScript was disabled
(again,
> > needs to be checked back with data analysts),
> > (c) disallow non-JavaScript visitors to submit the form or
> > (d) create the form fields from the GET parameters dynamically on the server
> > side.
> 
> I think if we can disable form submission for people without JS enabled we
will
> be saved from most of bots, anyway the only way for people to reach the form
is
> by direct link or from extension, the later one should also have JS enabled,
> unless there is no option enabling JS only for extensions.
> 
> Apart from this question would be great to have @kzar feedback under the
review
> as well about generating the hidden input content dynamically on server side,
> initially we decided to have it in JS, because of complication, but I'll be
> still fan of generating it on serverside (that's something we will probably
will
> need in future as well).

I suppose server-side generation of the form is off the table now, right?

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
File pages/uninstall-abp-submit.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp-submit.md:11: {{thanks-you[Thank you  message below heading]
Thank you for your participation}}
Detail: I guess you meant "thank-you" for the ID name.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:10: ["1v0", "{{reason-not-installed[Uninstallation
reason] I didn't install Adblock Plus.}}"],
This code may break if the translated text contains `"` so either we need to
make sure that we're not getting such translations or we sanitize the texts via
Python.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:37: for (var i = reasons.length; i > 0; i -= 1)
Detail: Actually, you don't even need the `i` variable if you write it like
this:

while (reasons.length)
{
  var reason = reasons.splice((Math.random() * reasons.length) >> 0, 1)[0];
  ...
}

Note: I just use the `>>` operator here because I personally prefer it over
`Math.floor` but no need to use it if you don't want to.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:39: var reason = reasons.splice(Math.floor(Math.random()
* i), 1)[0];
You should create a copy of `reasons` before the loop because removing elements
from the array while looping through it may cause issues.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
`reason[1]` doesn't appear to include any HTML so better use
`document.createTextNode()` instead of creating a `<span>` to avoid creating
unnecessary HTML elements.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:97: <button id="submit-form"
type="button">{{submit[Submit button text] Submit}}</button>
Detail: It's a `<button>` element so isn't the `type="button"` attribute
redundant?

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:101: {{disclaimer[Disclaimer below form] By clicking
Submit, you are sending your response to Adblock Plus. Please see our [privacy
policy](https://adblockplus.org/privacy).}}
Detail: This page is located on https://adblockplus.org so we should be able to
simply link to "privacy" instead.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:34: font-family: "Source Sans Pro";
Please specify fallback fonts in case of issues. In the new options page we use
`"Source Sans Pro", sans-serif`.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:62: -o-box-sizing: border-box;
I don't see a reason why any of those vendor prefixed versions should be
included. For now, this page will only be shown to Chrome users and even if we
decide to roll it out to other platforms, we no longer support any of the
platforms that requires a vendor prefix on this property.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:76: header:before
Detail: Same as the comment above. This would only be relevant for IE8 but so
far there's no indication that IE8 users will land on this page anytime soon.
Therefore I'd suggest using the "newer" `::before` syntax.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:200: form input[type="checkbox"] + span
I suppose that's a hack to avoid using `margin-end` on `input[type="checkbox"]`,
right?

https://codereview.adblockplus.org/29329984/diff/29331137/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29331137/templates/simple.tm...
templates/simple.tmpl:35: {{"impressum"|linkify}}{{"Legal
notice"|translate("legal-notice", "Legal notice in the footer")}}</a>
Definitely not a fan of this `linkify` function because it makes this line look
very confusing. Would be great if we could improve this at some point in the
future.

kzar

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md#newcode10 pages/uninstall-abp.md:10: ["1v0", "{{reason-not-installed[Uninstallation reason] I didn't install Adblock Plus.}}"], On ...

Nov. 28, 2015, 4:25 p.m. (2015-11-28 16:25:16 UTC) #23

saroyanm

New patch uploaded. https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp-submit.md File pages/uninstall-abp-submit.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp-submit.md#newcode11 pages/uninstall-abp-submit.md:11: {{thanks-you[Thank you message below heading] Thank ...

Nov. 30, 2015, 10:27 a.m. (2015-11-30 10:27:10 UTC) #24

New patch uploaded.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
File pages/uninstall-abp-submit.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp-submit.md:11: {{thanks-you[Thank you  message below heading]
Thank you for your participation}}
On 2015/11/27 19:10:11, Thomas Greiner wrote:
> Detail: I guess you meant "thank-you" for the ID name.

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:37: for (var i = reasons.length; i > 0; i -= 1)
On 2015/11/27 19:10:11, Thomas Greiner wrote:
> Detail: Actually, you don't even need the `i` variable if you write it like
> this:
> 
> while (reasons.length)
> {
>   var reason = reasons.splice((Math.random() * reasons.length) >> 0, 1)[0];
>   ...
> }
> 
> Note: I just use the `>>` operator here because I personally prefer it over
> `Math.floor` but no need to use it if you don't want to.

Please check current discussion, I'm also in favor of using while:
https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...

If you also think the initial solution is better will be happy to switch to
having while, hope Dave do not have objections.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
On 2015/11/27 19:10:13, Thomas Greiner wrote:
> `reason[1]` doesn't appear to include any HTML so better use
> `document.createTextNode()` instead of creating a `<span>` to avoid creating
> unnecessary HTML elements.

The reason why I used span, is that I wanted the text element to have both left
and right margins and keep the input aligned to the start of it's parent
element, otherwise I'll need to have both margins for input element and adjust
all other elements in the page according to the margin I've set to input
element, I thought having that margin for the sibling element of input will be
better solution, also I used innerHTML to convert escaped characters like "'".

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:97: <button id="submit-form"
type="button">{{submit[Submit button text] Submit}}</button>
On 2015/11/27 19:10:12, Thomas Greiner wrote:
> Detail: It's a `<button>` element so isn't the `type="button"` attribute
> redundant?

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:101: {{disclaimer[Disclaimer below form] By clicking
Submit, you are sending your response to Adblock Plus. Please see our [privacy
policy](https://adblockplus.org/privacy).}}
On 2015/11/27 19:10:12, Thomas Greiner wrote:
> Detail: This page is located on https://adblockplus.org so we should be able
to
> simply link to "privacy" instead.

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:34: font-family: "Source Sans Pro";
On 2015/11/27 19:10:14, Thomas Greiner wrote:
> Please specify fallback fonts in case of issues. In the new options page we
use
> `"Source Sans Pro", sans-serif`.

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:62: -o-box-sizing: border-box;
On 2015/11/27 19:10:15, Thomas Greiner wrote:
> I don't see a reason why any of those vendor prefixed versions should be
> included. For now, this page will only be shown to Chrome users and even if we
> decide to roll it out to other platforms, we no longer support any of the
> platforms that requires a vendor prefix on this property.

Fare enough.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:76: header:before
On 2015/11/27 19:10:15, Thomas Greiner wrote:
> Detail: Same as the comment above. This would only be relevant for IE8 but so
> far there's no indication that IE8 users will land on this page anytime soon.
> Therefore I'd suggest using the "newer" `::before` syntax.

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:200: form input[type="checkbox"] + span
On 2015/11/27 19:10:14, Thomas Greiner wrote:
> I suppose that's a hack to avoid using `margin-end` on
`input[type="checkbox"]`,
> right?

Exactly, but do not have strong opinion, if we will use margin-end we can also
think about having textNode instead of span for alignment.

https://codereview.adblockplus.org/29329984/diff/29331137/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29331137/templates/simple.tm...
templates/simple.tmpl:35: {{"impressum"|linkify}}{{"Legal
notice"|translate("legal-notice", "Legal notice in the footer")}}</a>
On 2015/11/27 19:10:16, Thomas Greiner wrote:
> Definitely not a fan of this `linkify` function because it makes this line
look
> very confusing. Would be great if we could improve this at some point in the
> future.

You mean having something similar to example below for future ?:
<a href="{{'impressum'|linkify}}">..</a>

kzar

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md#newcode37 pages/uninstall-abp.md:37: for (var i = reasons.length; i > 0; i ...

Nov. 30, 2015, 10:49 a.m. (2015-11-30 10:49:11 UTC) #25

Thomas Greiner

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md#newcode10 pages/uninstall-abp.md:10: ["1v0", "{{reason-not-installed[Uninstallation reason] I didn't install Adblock Plus.}}"], On ...

Nov. 30, 2015, 5:09 p.m. (2015-11-30 17:09:14 UTC) #26

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:10: ["1v0", "{{reason-not-installed[Uninstallation
reason] I didn't install Adblock Plus.}}"],
On 2015/11/28 16:25:14, kzar wrote:
> On 2015/11/27 19:10:13, Thomas Greiner wrote:
> > This code may break if the translated text contains `"` so either we need to
> > make sure that we're not getting such translations or we sanitize the texts
> via
> > Python.
> 
> No, I already checked and they are escaped. Just to make sure I checked again:
> 
> <head>
>   <meta name="robots" content="noindex" />
>   <script type="text/javascript">
>    var foo = "{{foo "oo}}";
>    var bar = '{{bar bar'}}';
>    console.log(foo, bar);
>   </script>
> </head>
> 
> => &quot;oo bar&#39;

Thanks for checking. Should be fine then.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:37: for (var i = reasons.length; i > 0; i -= 1)
On 2015/11/30 10:49:10, kzar wrote:
> On 2015/11/30 10:27:05, saroyanm wrote:
> > On 2015/11/27 19:10:11, Thomas Greiner wrote:
> > > Detail: Actually, you don't even need the `i` variable if you write it
like
> > > this:
> > > 
> > > while (reasons.length)
> > > {
> > >   var reason = reasons.splice((Math.random() * reasons.length) >> 0,
1)[0];
> > >   ...
> > > }
> > > 
> > > Note: I just use the `>>` operator here because I personally prefer it
over
> > > `Math.floor` but no need to use it if you don't want to.
> > 
> > Please check current discussion, I'm also in favor of using while:
> >
>
https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
> > 
> > If you also think the initial solution is better will be happy to switch to
> > having while, hope Dave do not have objections.
> 
> I don't mind at all how this code is implemented, I was just trying to help
with
> my suggestion before. My only request is that if you change this please test
> that it works as expected properly, it will mess up the data otherwise.

I would consider the original solution simpler than the current one. The
for-loop makes it look like you're looping through the array, while the
while-loop makes it more clear that you're just looping until the array is
empty.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:39: var reason = reasons.splice(Math.floor(Math.random()
* i), 1)[0];
On 2015/11/28 16:25:14, kzar wrote:
> On 2015/11/27 19:10:12, Thomas Greiner wrote:
> > You should create a copy of `reasons` before the loop because removing
> elements
> > from the array while looping through it may cause issues.
> 
> Well we're not looping through the reasons array, we're looping down from its
> original length to > 0. I suggested this for loop approach to Manvel and
before
> I did I carefully tested that it worked properly. (See our earlier
discussion.)
> 
> I don't mind if we change it to something better if you guys can think of
> something, but please check it produces n! groups with a fairly even
> distribution. This is the kind of logic that is very easy to miss bugs in.

Must've forgotten to remove that comment after I realized that `i` is not the
index but the array length. At least it's showcasing nicely what confusion the
for-loop can cause. :D

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
On 2015/11/30 10:27:05, saroyanm wrote:
> On 2015/11/27 19:10:13, Thomas Greiner wrote:
> > `reason[1]` doesn't appear to include any HTML so better use
> > `document.createTextNode()` instead of creating a `<span>` to avoid creating
> > unnecessary HTML elements.
> 
> The reason why I used span, is that I wanted the text element to have both
left
> and right margins and keep the input aligned to the start of it's parent
> element, otherwise I'll need to have both margins for input element and adjust
> all other elements in the page according to the margin I've set to input
> element, I thought having that margin for the sibling element of input will be
> better solution,

That appears a bit odd given that the style guide doesn't mention any margins on
the right.

> also I used innerHTML to convert escaped characters like "'".

So a workaround for a workaround? ;) Why not simply create the input fields in
the HTML and only move them inside the form via JavaScript? Then we could treat
it like any other HTML element and don't need to construct them via JavaScript.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:62: -o-box-sizing: border-box;
On 2015/11/30 10:27:07, saroyanm wrote:
> On 2015/11/27 19:10:15, Thomas Greiner wrote:
> > I don't see a reason why any of those vendor prefixed versions should be
> > included. For now, this page will only be shown to Chrome users and even if
we
> > decide to roll it out to other platforms, we no longer support any of the
> > platforms that requires a vendor prefix on this property.
> 
> Fare enough.

Again: "we no longer support any of the
platforms that requires a vendor prefix on this property"

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:200: form input[type="checkbox"] + span
On 2015/11/30 10:27:08, saroyanm wrote:
> On 2015/11/27 19:10:14, Thomas Greiner wrote:
> > I suppose that's a hack to avoid using `margin-end` on
> `input[type="checkbox"]`,
> > right?
> 
> Exactly, but do not have strong opinion, if we will use margin-end we can also
> think about having textNode instead of span for alignment.

That's what I'd suggest, yes, to try to apply styles to elements for which it
makes the most sense - especially if we can avoid introducing new elements for
the sole purpose of applying styles to them.

saroyanm

New patch uploaded. https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md#newcode37 pages/uninstall-abp.md:37: for (var i = reasons.length; i ...

Nov. 30, 2015, 6:04 p.m. (2015-11-30 18:04:50 UTC) #27

New patch uploaded.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:37: for (var i = reasons.length; i > 0; i -= 1)
On 2015/11/30 17:09:06, Thomas Greiner wrote:
> On 2015/11/30 10:49:10, kzar wrote:
> > On 2015/11/30 10:27:05, saroyanm wrote:
> > > On 2015/11/27 19:10:11, Thomas Greiner wrote:
> > > > Detail: Actually, you don't even need the `i` variable if you write it
> like
> > > > this:
> > > > 
> > > > while (reasons.length)
> > > > {
> > > >   var reason = reasons.splice((Math.random() * reasons.length) >> 0,
> 1)[0];
> > > >   ...
> > > > }
> > > > 
> > > > Note: I just use the `>>` operator here because I personally prefer it
> over
> > > > `Math.floor` but no need to use it if you don't want to.
> > > 
> > > Please check current discussion, I'm also in favor of using while:
> > >
> >
>
https://codereview.adblockplus.org/29329984/diff/29330503/pages/uninstall-abp...
> > > 
> > > If you also think the initial solution is better will be happy to switch
to
> > > having while, hope Dave do not have objections.
> > 
> > I don't mind at all how this code is implemented, I was just trying to help
> with
> > my suggestion before. My only request is that if you change this please test
> > that it works as expected properly, it will mess up the data otherwise.
> 
> I would consider the original solution simpler than the current one. The
> for-loop makes it look like you're looping through the array, while the
> while-loop makes it more clear that you're just looping until the array is
> empty.

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
On 2015/11/30 17:09:07, Thomas Greiner wrote:
> On 2015/11/30 10:27:05, saroyanm wrote:
> > On 2015/11/27 19:10:13, Thomas Greiner wrote:
> > > `reason[1]` doesn't appear to include any HTML so better use
> > > `document.createTextNode()` instead of creating a `<span>` to avoid
creating
> > > unnecessary HTML elements.
> > 
> > The reason why I used span, is that I wanted the text element to have both
> left
> > and right margins and keep the input aligned to the start of it's parent
> > element, otherwise I'll need to have both margins for input element and
adjust
> > all other elements in the page according to the margin I've set to input
> > element, I thought having that margin for the sibling element of input will
be
> > better solution,
> 
> That appears a bit odd given that the style guide doesn't mention any margins
on
> the right.
>
> > also I used innerHTML to convert escaped characters like "'".
> 
> So a workaround for a workaround? ;) Why not simply create the input fields in
> the HTML and only move them inside the form via JavaScript? Then we could
treat
> it like any other HTML element and don't need to construct them via
JavaScript.

I guess we initially implemented something similar, but decided to generate all
the elements in JavaScript, because at that time we thought it's better to
create elements in JS rather than declare and remove/move them from the HTML,
but currently I do not have strong opinion, but on other hand implementation
similar to the original one will justify usage of innerHTML here, I mean we can
just skip it:
https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:62: -o-box-sizing: border-box;
On 2015/11/30 17:09:08, Thomas Greiner wrote:
> On 2015/11/30 10:27:07, saroyanm wrote:
> > On 2015/11/27 19:10:15, Thomas Greiner wrote:
> > > I don't see a reason why any of those vendor prefixed versions should be
> > > included. For now, this page will only be shown to Chrome users and even
if
> we
> > > decide to roll it out to other platforms, we no longer support any of the
> > > platforms that requires a vendor prefix on this property.
> > 
> > Fare enough.
> 
> Again: "we no longer support any of the
> platforms that requires a vendor prefix on this property"

Hmm, for some reason I was thinking we should use -webkit, and -moz while this
is experimental technology :/ 
Done.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:200: form input[type="checkbox"] + span
On 2015/11/30 17:09:13, Thomas Greiner wrote:
> On 2015/11/30 10:27:08, saroyanm wrote:
> > On 2015/11/27 19:10:14, Thomas Greiner wrote:
> > > I suppose that's a hack to avoid using `margin-end` on
> > `input[type="checkbox"]`,
> > > right?
> > 
> > Exactly, but do not have strong opinion, if we will use margin-end we can
also
> > think about having textNode instead of span for alignment.
> 
> That's what I'd suggest, yes, to try to apply styles to elements for which it
> makes the most sense - especially if we can avoid introducing new elements for
> the sole purpose of applying styles to them.

Just rethought about this question, while this is a page that probably will be
used for other pages and probably in future we will reuse this page for IE
users, does make sense to use styles that will be supported also for them.

Thomas Greiner

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md#newcode47 pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1]; On 2015/11/30 18:04:43, saroyanm wrote: > ...

Dec. 2, 2015, 4:41 p.m. (2015-12-02 16:41:07 UTC) #28

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
On 2015/11/30 18:04:43, saroyanm wrote:
> On 2015/11/30 17:09:07, Thomas Greiner wrote:
> > On 2015/11/30 10:27:05, saroyanm wrote:
> > > On 2015/11/27 19:10:13, Thomas Greiner wrote:
> > > > `reason[1]` doesn't appear to include any HTML so better use
> > > > `document.createTextNode()` instead of creating a `<span>` to avoid
> creating
> > > > unnecessary HTML elements.
> > > 
> > > The reason why I used span, is that I wanted the text element to have both
> > left
> > > and right margins and keep the input aligned to the start of it's parent
> > > element, otherwise I'll need to have both margins for input element and
> adjust
> > > all other elements in the page according to the margin I've set to input
> > > element, I thought having that margin for the sibling element of input
will
> be
> > > better solution,
> > 
> > That appears a bit odd given that the style guide doesn't mention any
margins
> on
> > the right.
> >
> > > also I used innerHTML to convert escaped characters like "'".
> > 
> > So a workaround for a workaround? ;) Why not simply create the input fields
in
> > the HTML and only move them inside the form via JavaScript? Then we could
> treat
> > it like any other HTML element and don't need to construct them via
> JavaScript.
> 
> I guess we initially implemented something similar, but decided to generate
all
> the elements in JavaScript, because at that time we thought it's better to
> create elements in JS rather than declare and remove/move them from the HTML,
> but currently I do not have strong opinion, but on other hand implementation
> similar to the original one will justify usage of innerHTML here, I mean we
can
> just skip it:
>
https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...

Personally, I'd prefer avoid using `innerHTML` and dynamic generation of static
HTML elements. Therefore I'd suggest that change but leave it up to you to
decide which one to go with.

I'd say that the best JavaScript developer is someone who is able to avoid
writing JavaScript code (see also this tweet from the creator of jQuery
https://twitter.com/jeresig/status/671832021837193216).

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
In case you missed it from the comment above…

On 2015/11/30 10:27:05, saroyanm wrote:
> On 2015/11/27 19:10:13, Thomas Greiner wrote:
> > `reason[1]` doesn't appear to include any HTML so better use
> > `document.createTextNode()` instead of creating a `<span>` to avoid creating
> > unnecessary HTML elements.
> 
> The reason why I used span, is that I wanted the text element to have both
left
> and right margins and keep the input aligned to the start of it's parent
> element, otherwise I'll need to have both margins for input element and adjust
> all other elements in the page according to the margin I've set to input
> element, I thought having that margin for the sibling element of input will be
> better solution,

That appears a bit odd given that the style guide doesn't mention any margins on
the right.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:200: form input[type="checkbox"] + span
On 2015/11/30 18:04:44, saroyanm wrote:
> On 2015/11/30 17:09:13, Thomas Greiner wrote:
> > On 2015/11/30 10:27:08, saroyanm wrote:
> > > On 2015/11/27 19:10:14, Thomas Greiner wrote:
> > > > I suppose that's a hack to avoid using `margin-end` on
> > > `input[type="checkbox"]`,
> > > > right?
> > > 
> > > Exactly, but do not have strong opinion, if we will use margin-end we can
> also
> > > think about having textNode instead of span for alignment.
> > 
> > That's what I'd suggest, yes, to try to apply styles to elements for which
it
> > makes the most sense - especially if we can avoid introducing new elements
for
> > the sole purpose of applying styles to them.
> 
> Just rethought about this question, while this is a page that probably will be
> used for other pages and probably in future we will reuse this page for IE
> users, does make sense to use styles that will be supported also for them.

Let's not make our lives more complicated than necessary especially since we
don't know when or whether we'll show that page to IE users. Obviously, we
shouldn't use anything that we know will require significant changes to make
backwards-compatible but IE-support is not a requirement at this point.

https://codereview.adblockplus.org/29329984/diff/29331658/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331658/pages/uninstall-abp...
pages/uninstall-abp.md:39: var randomIndex = Math.floor(Math.random() *
(reasons.length));
Detail: Unnecessary brackets around `reasons.length`.

saroyanm

New patches uploaded. https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md File pages/uninstall-abp.md (right): https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md#newcode47 pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1]; On 2015/12/02 16:41:04, ...

Dec. 2, 2015, 6:48 p.m. (2015-12-02 18:48:00 UTC) #29

New patches uploaded.

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp.md
File pages/uninstall-abp.md (right):

https://codereview.adblockplus.org/29329984/diff/29331137/pages/uninstall-abp...
pages/uninstall-abp.md:47: spanElement.innerHTML = reason[1];
On 2015/12/02 16:41:04, Thomas Greiner wrote:
> On 2015/11/30 18:04:43, saroyanm wrote:
> > On 2015/11/30 17:09:07, Thomas Greiner wrote:
> > > On 2015/11/30 10:27:05, saroyanm wrote:
> > > > On 2015/11/27 19:10:13, Thomas Greiner wrote:
> > > > > `reason[1]` doesn't appear to include any HTML so better use
> > > > > `document.createTextNode()` instead of creating a `<span>` to avoid
> > creating
> > > > > unnecessary HTML elements.
> > > > 
> > > > The reason why I used span, is that I wanted the text element to have
both
> > > left
> > > > and right margins and keep the input aligned to the start of it's parent
> > > > element, otherwise I'll need to have both margins for input element and
> > adjust
> > > > all other elements in the page according to the margin I've set to input
> > > > element, I thought having that margin for the sibling element of input
> will
> > be
> > > > better solution,
> > > 
> > > That appears a bit odd given that the style guide doesn't mention any
> margins
> > on
> > > the right.
> > >
> > > > also I used innerHTML to convert escaped characters like "'".
> > > 
> > > So a workaround for a workaround? ;) Why not simply create the input
fields
> in
> > > the HTML and only move them inside the form via JavaScript? Then we could
> > treat
> > > it like any other HTML element and don't need to construct them via
> > JavaScript.
> > 
> > I guess we initially implemented something similar, but decided to generate
> all
> > the elements in JavaScript, because at that time we thought it's better to
> > create elements in JS rather than declare and remove/move them from the
HTML,
> > but currently I do not have strong opinion, but on other hand implementation
> > similar to the original one will justify usage of innerHTML here, I mean we
> can
> > just skip it:
> >
>
https://codereview.adblockplus.org/29329984/diff/29330307/pages/uninstall-abp...
> 
> Personally, I'd prefer avoid using `innerHTML` and dynamic generation of
static
> HTML elements. Therefore I'd suggest that change but leave it up to you to
> decide which one to go with.
> 
> I'd say that the best JavaScript developer is someone who is able to avoid
> writing JavaScript code (see also this tweet from the creator of jQuery
> https://twitter.com/jeresig/status/671832021837193216).

Done.

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.css
File static/css/simple.css (right):

https://codereview.adblockplus.org/29329984/diff/29331137/static/css/simple.c...
static/css/simple.css:200: form input[type="checkbox"] + span
On 2015/12/02 16:41:04, Thomas Greiner wrote:
> On 2015/11/30 18:04:44, saroyanm wrote:
> > On 2015/11/30 17:09:13, Thomas Greiner wrote:
> > > On 2015/11/30 10:27:08, saroyanm wrote:
> > > > On 2015/11/27 19:10:14, Thomas Greiner wrote:
> > > > > I suppose that's a hack to avoid using `margin-end` on
> > > > `input[type="checkbox"]`,
> > > > > right?
> > > > 
> > > > Exactly, but do not have strong opinion, if we will use margin-end we
can
> > also
> > > > think about having textNode instead of span for alignment.
> > > 
> > > That's what I'd suggest, yes, to try to apply styles to elements for which
> it
> > > makes the most sense - especially if we can avoid introducing new elements
> for
> > > the sole purpose of applying styles to them.
> > 
> > Just rethought about this question, while this is a page that probably will
be
> > used for other pages and probably in future we will reuse this page for IE
> > users, does make sense to use styles that will be supported also for them.
> 
> Let's not make our lives more complicated than necessary especially since we
> don't know when or whether we'll show that page to IE users. Obviously, we
> shouldn't use anything that we know will require significant changes to make
> backwards-compatible but IE-support is not a requirement at this point.

Done.

Thomas Greiner

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl#newcode38 pages/uninstall-abp.tmpl:38: var reasons = reasonsContainer.cloneNode(true).getElementsByTagName("li"); Why do you clone the ...

Dec. 3, 2015, 12:07 p.m. (2015-12-03 12:07:32 UTC) #30

saroyanm

New patch uploaded. https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl#newcode38 pages/uninstall-abp.tmpl:38: var reasons = reasonsContainer.cloneNode(true).getElementsByTagName("li"); On 2015/12/03 ...

Dec. 3, 2015, 1:07 p.m. (2015-12-03 13:07:05 UTC) #31

New patch uploaded.

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp...
File pages/uninstall-abp.tmpl (right):

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp...
pages/uninstall-abp.tmpl:38: var reasons =
reasonsContainer.cloneNode(true).getElementsByTagName("li");
On 2015/12/03 12:07:28, Thomas Greiner wrote:
> Why do you clone the container? All you seem to do with it is get a list of
its
> children which you can do without cloning it.

I'm removing the lists from the HTML, that means the relation is lost and I'll
end up with empty array, so that why I'm cloning.

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp...
pages/uninstall-abp.tmpl:38: var reasons =
reasonsContainer.cloneNode(true).getElementsByTagName("li");
On 2015/12/03 12:07:28, Thomas Greiner wrote:
> `reasons` will also include `#reason-other` which should always stay at the
end
> of the list.

Yes that why I'm using "reasons.length - 1" for generating random number it will
left the last element untouched.

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp...
pages/uninstall-abp.tmpl:40: var reasons = Array.prototype.slice.call(reasons);
On 2015/12/03 12:07:29, Thomas Greiner wrote:
> `reasons` has already been declared so there shouldn't be a `var` here.

Done.

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp...
pages/uninstall-abp.tmpl:43: var randomIndex = Math.floor(Math.random() *
(reasons.length -1));
On 2015/12/03 12:07:30, Thomas Greiner wrote:
> Why did you change it to `reasons.length - 1`? The output of `Math.random()`
> will always be a number between 0 (inclusive) and 1 (exclusive) so
`randomIndex`
> will always be less than the value the random number is multiplied with due to
> `Math.floor()`.

Sure, please see my comment above I'm doing so to leave the last element and on
last iteration add it.

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp...
pages/uninstall-abp.tmpl:85: <span>{{value|translate(stringId, "Uninstallation
reason")}}</span>
On 2015/12/03 12:07:29, Thomas Greiner wrote:
> I thought there's no need for having a `<span>` here anymore.

Ahh right, totally forgot, thanks for the reminder.

Thomas Greiner

https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl#newcode38 pages/uninstall-abp.tmpl:38: var reasons = reasonsContainer.cloneNode(true).getElementsByTagName("li"); On 2015/12/03 13:06:57, saroyanm wrote: ...

Dec. 3, 2015, 1:47 p.m. (2015-12-03 13:47:38 UTC) #32

saroyanm

New patch uploaded. https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl File pages/uninstall-abp.tmpl (right): https://codereview.adblockplus.org/29329984/diff/29331796/pages/uninstall-abp.tmpl#newcode38 pages/uninstall-abp.tmpl:38: var reasons = reasonsContainer.cloneNode(true).getElementsByTagName("li"); On 2015/12/03 ...

Dec. 3, 2015, 1:57 p.m. (2015-12-03 13:57:11 UTC) #33

LGTM

Issue 29329984: Issue 3257 - Create uninstallation page in adblockplus.org (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed Thomas initiall comments #

Patch Set 3 : Rename logo to make it consistent #

Patch Set 4 : Addressed Dave comments #

Patch Set 5 : Removed the shuffling filter #

Patch Set 6 : Rename jinja files to markdown #

Patch Set 7 : Fix old jinja files compatibility with markdown and implement noscript #

Patch Set 8 : Removed noscript notification #

Patch Set 9 : renamed merkurial files to template #

Patch Set 10 : Removed section from template #

Patch Set 11 : Fix editor readability, use for loop to randomly generate the list #

Patch Set 12 : rename template files again back to markdown #

Patch Set 13 : move include files content to markdown #

Patch Set 14 : Addressed Dave comments #

Patch Set 15 : Addressed Thomas comments #

Patch Set 16 : Switched to old implementation of shuffling using while #

Patch Set 17 : renamed .md file back to .tmpl #

Patch Set 18 : Generate reasons in HTML then shuffle them #

Patch Set 19 : Addressed Thomas comments #

Patch Set 20 : Convert to array instead of cloning #

Messages