pages/uninstall-abp.tmpl - Issue 29329984: Issue 3257 - Create uninstallation page in adblockplus.org

Side by Side Diff: pages/uninstall-abp.tmpl

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

Patch Set: Generate reasons in HTML then shuffle them Created Dec. 2, 2015, 6:43 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 title=Adblock Plus has been uninstalled

	2 template=simple

	3

	4 {% set reasons = [

	5 ("1v0", "reason-not-installed", "I didn't install Adblock Plus."),

	6 ("2v0", "reason-slowing-down", "Adblock Plus slowed down my browser."),

	7 ("3v0", "reason-acceptable-ads", "I don't like the Acceptable Ads program."),

	8 ("4v0", "reason-see-ads", "Adblock Plus didn't block all ads."),

	9 ("5v0", "reason-better-adblocker", "I found better ad blocking software."),

	10 ("6v0", "reason-break-websites", "Adblock Plus breaks websites that I visit.")

	11 ] %}

	12

	13 <head>

	14 <meta name="robots" content="noindex" />

	15 <script type="text/javascript">

	16 (function()

	17 {

	18 function init()

	19 {

	20 var form = document.getElementById("reasons-form");

	21

	22 // Create hidden input for GET parameters

	23 window.location.search.substr(1).split("&").forEach(function(param)

	24 {

	25 if (!/.=./.test(param))

	26 return;

	27

	28 var paramSplit = param.split("=");

	29 var input = document.createElement("input");

	30 input.setAttribute("type", "hidden");

	31 input.setAttribute("name", paramSplit[0]);

	32 input.setAttribute("value", paramSplit[1]);

	33 form.appendChild(input);

	34 });

	35

	36 // Randomly add reasons

	37 var reasonsContainer = document.getElementById("reasons");

	38 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 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 `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 t Show quoted text 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. saroyanm 2015/12/03 13:06:57 Yes that why I'm using "reasons.length - 1" for ge Show quoted text 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. Thomas Greiner 2015/12/03 13:47:37 Ok. Show quoted text On 2015/12/03 13:06:57, saroyanm wrote: > 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. Ok. Thomas Greiner 2015/12/03 13:47:37 You can keep the references to the DOM elements in Show quoted text On 2015/12/03 13:06:57, saroyanm wrote: > 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. 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 :) Show quoted text On 2015/12/03 13:47:37, Thomas Greiner wrote: > On 2015/12/03 13:06:57, saroyanm wrote: > > 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. > > 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. Fare enough :)
	39 reasonsContainer.innerHTML = "";

	40 var reasons = Array.prototype.slice.call(reasons);
	Thomas Greiner 2015/12/03 12:07:29 `reasons` has already been declared so there shoul `reasons` has already been declared so there shouldn't be a `var` here. saroyanm 2015/12/03 13:06:57 Done. Show quoted text On 2015/12/03 12:07:29, Thomas Greiner wrote: > `reasons` has already been declared so there shouldn't be a `var` here. Done.
	41 while (reasons.length)

	42 {

	43 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 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 Show quoted text 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.
	44 var reasonElement = reasons.splice(randomIndex, 1)[0];

	45 reasonsContainer.appendChild(reasonElement);

	46 }

	47

	48 var reasonOtherCheckbox = document.getElementById("reason-other");

	49 reasonOtherCheckbox.addEventListener("change", function()

	50 {

	51 var textArea = document.getElementById("reason-other-input");

	52 if (textArea.hasAttribute("class"))

	53 textArea.removeAttribute("class")

	54 else

	55 textArea.setAttribute("class", "hidden");

	56 }, false);

	57

	58 var submitButton = document.getElementById("submit-form");

	59 submitButton.addEventListener("click", function(event)

	60 {

	61 if (!document.querySelector("ul input:checked"))

	62 {

	63 event.preventDefault();

	64 form.setAttribute("class", "error");

	65 }

	66 else

	67 {

	68 form.submit();

	69 }

	70 }, false);

	71 }

	72 document.addEventListener("DOMContentLoaded", init, false);

	73 })();

	74 </script>

	75 </head>

	76

	77 <section class="highlighted">

	78 <h1>{{"Please select the reason(s) why you uninstalled Adblock Plus:"\|translat e("reasons-header", "Form heading")}}</h1>

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

	80 <ul id="reasons">

	81 {%- for reasonId, stringId, value in reasons %}

	82 <li>

	83 <label>

	84 <input type="checkbox" name="reason" value="{{reasonId}}" />

	85 <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>` he 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 Show quoted text 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.
	86 </label>

	87 </li>

	88 {%- endfor %}

	89 <li>

	90 <label>

	91 <input id="reason-other" type="checkbox" name="reason" value="0v0" />

	92 <span>{{"Other, namely..."\|translate("reason-other", "Last uninstallat ion reason")}}</span>

	93 </label>

	94 <textarea id="reason-other-input" class="hidden" name="reasonOther" plac eholder="{{"Please explain why you are uninstalling Adblock Plus"\|translate("rea son-other-placeholder", "Textarea placeholder text, appears after selecting 'Oth er, namely...' option")}}"></textarea>

	95 </li>

	96 </ul>

	97 <button id="submit-form">{{"Submit"\|translate("submit", "Submit button text" )}}</button>

	98 <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 i tem selected")}}</span>

	99 </form>

	100 <p class="disclaimer">

	101 {{"By clicking Submit, you are sending your response to Adblock Plus. Please see our <a href='/privacy'>privacy policy</a>."\|translate("disclaimer", "Discla imer below form")}}

	102 </p>

	103 </section>

OLD	NEW

« no previous file with comments | « no previous file | pages/uninstall-abp-submit.md » ('j') | no next file with comments »