Issue 3257 - Create uninstallation page in adblockplus.org

pages/uninstall-abp.tmpl

 title=Adblock Plus has been uninstalled
 template=simple
 
 {% set 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.")
@@ skipping 17 matching lines @@
         var paramSplit = param.split("=");
         var input = document.createElement("input");
         input.setAttribute("type", "hidden");
         input.setAttribute("name", paramSplit[0]);
         input.setAttribute("value", paramSplit[1]);
         form.appendChild(input);
       });
 
       // Randomly add reasons
       var reasonsContainer = document.getElementById("reasons");
-      var reasons = reasonsContainer.cloneNode(true).getElementsByTagName("li");

[Thomas Greiner, 2015/12/03 12:07:28]

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.

[Thomas Greiner, 2015/12/03 12:07:28]

`reasons` will also include `#reason-other` which should always stay at the end
of the list.

[saroyanm, 2015/12/03 13:06:57]

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.

[saroyanm, 2015/12/03 13:06:57]

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

[Thomas Greiner, 2015/12/03 13:47:37]

Ok.

[Thomas Greiner, 2015/12/03 13:47:37]

You can keep the references to the DOM elements intact by converting it into an
array before modifying the container content instead of after. Thereby you don't
need to clone the parent element because you're no longer working with a
`HTMLCollection`. Only those are automatically changing whenever the document
changes.

[saroyanm, 2015/12/03 13:57:10]

Fare enough :)

+      var reasons = reasonsContainer.getElementsByTagName("li");
+      reasons = Array.prototype.slice.call(reasons);
       reasonsContainer.innerHTML = "";
-      var reasons = Array.prototype.slice.call(reasons);

[Thomas Greiner, 2015/12/03 12:07:29]

`reasons` has already been declared so there shouldn't be a `var` here.

[saroyanm, 2015/12/03 13:06:57]

Done.

       while (reasons.length)
       {
         var randomIndex = Math.floor(Math.random() * (reasons.length -1));

[Thomas Greiner, 2015/12/03 12:07:30]

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()`.

[saroyanm, 2015/12/03 13:06:58]

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

         var reasonElement = reasons.splice(randomIndex, 1)[0];
         reasonsContainer.appendChild(reasonElement);
       }
 
       var reasonOtherCheckbox = document.getElementById("reason-other");
       reasonOtherCheckbox.addEventListener("change", function()
       {
         var textArea = document.getElementById("reason-other-input");
         if (textArea.hasAttribute("class"))
           textArea.removeAttribute("class")
@@ skipping 21 matching lines @@
 </head>
 
 <section class="highlighted">
   <h1>{{"Please select the reason(s) why you uninstalled Adblock Plus:"|translate("reasons-header", "Form heading")}}</h1>
   <form id="reasons-form" action="uninstall-abp-submit" method="post">
     <ul id="reasons">
       {%- for reasonId, stringId, value in reasons %}
       <li>
         <label>
           <input type="checkbox" name="reason" value="{{reasonId}}" />
-          <span>{{value|translate(stringId, "Uninstallation reason")}}</span>

[Thomas Greiner, 2015/12/03 12:07:29]

I thought there's no need for having a `<span>` here anymore.

[saroyanm, 2015/12/03 13:06:59]

Ahh right, totally forgot, thanks for the reminder.

+          {{value|translate(stringId, "Uninstallation reason")}}
         </label>
       </li>
       {%- endfor %}
       <li>
         <label>
           <input id="reason-other" type="checkbox" name="reason" value="0v0" />
-          <span>{{"Other, namely..."|translate("reason-other", "Last uninstallation reason")}}</span>
+          {{"Other, namely..."|translate("reason-other", "Last uninstallation reason")}}
         </label>
         <textarea id="reason-other-input" class="hidden" name="reasonOther" placeholder="{{"Please explain why you are uninstalling Adblock Plus"|translate("reason-other-placeholder", "Textarea placeholder text, appears after selecting 'Other, namely...' option")}}"></textarea>
       </li>
     </ul>
     <button id="submit-form">{{"Submit"|translate("submit", "Submit button text")}}</button>
     <span class="error-label">{{"Please select at least one of the options above"|translate("error-msg", "Error message, is being shown after submission if no item selected")}}</span>
   </form>
   <p class="disclaimer">
     {{"By clicking Submit, you are sending your response to Adblock Plus. Please see our <a href='/privacy'>privacy policy</a>."|translate("disclaimer", "Disclaimer below form")}}
   </p>
 </section>