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

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

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

Patch Set: Addressed Dave comments Created Nov. 27, 2015, 2:25 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 <head>

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

	6 <script type="text/javascript">

	7 (function()

	8 {

	9 var reasons = [

	10 ["1v0", "{{reason-not-installed[Uninstallation reason] I didn't install Ad block Plus.}}"],
	Thomas Greiner 2015/11/27 19:10:13 This code may break if the translated text contain 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. kzar 2015/11/28 16:25:14 No, I already checked and they are escaped. Just t Show quoted text 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> => "oo bar' Thomas Greiner 2015/11/30 17:09:07 Thanks for checking. Should be fine then. Show quoted text 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> > > => "oo bar' Thanks for checking. Should be fine then.
	11 ["2v0", "{{reason-slowing-down[Uninstallation reason] Adblock Plus slowed down my browser.}}"],

	12 ["3v0", "{{reason-acceptable-ads[Uninstallation reason] I don't like the A cceptable Ads program.}}"],

	13 ["4v0", "{{reason-see-ads[Uninstallation reason] Adblock Plus didn't block all ads.}}"],

	14 ["5v0", "{{reason-better-adblocker[Uninstallation reason] I found better a d blocking software.}}"],

	15 ["6v0", "{{reason-break-websites[Uninstallation reason] Adblock Plus break s websites that I visit.}}"]

	16 ];

	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 for (var i = reasons.length; i > 0; i -= 1)
	Thomas Greiner 2015/11/27 19:10:11 Detail: Actually, you don't even need the `i` vari 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. saroyanm 2015/11/30 10:27:05 Please check current discussion, I'm also in favor Show quoted text 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. kzar 2015/11/30 10:49:10 I don't mind at all how this code is implemented, Show quoted text 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. Thomas Greiner 2015/11/30 17:09:06 I would consider the original solution simpler tha Show quoted text 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. saroyanm 2015/11/30 18:04:43 Done. Show quoted text 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.
	38 {

	39 var reason = reasons.splice(Math.floor(Math.random() * i), 1)[0];
	Thomas Greiner 2015/11/27 19:10:12 You should create a copy of `reasons` before the l You should create a copy of `reasons` before the loop because removing elements from the array while looping through it may cause issues. kzar 2015/11/28 16:25:14 Well we're not looping through the reasons array, Show quoted text 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. Thomas Greiner 2015/11/30 17:09:06 Must've forgotten to remove that comment after I r Show quoted text 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
	40 var listElement = document.createElement("li");

	41 var labelElement = document.createElement("label");

	42 var checkboxElement = document.createElement("input");

	43 checkboxElement.setAttribute("type", "checkbox");

	44 checkboxElement.setAttribute("name", "reason");

	45 checkboxElement.setAttribute("value", reason[0]);

	46 var spanElement = document.createElement("span");

	47 spanElement.innerHTML = reason[1];
	Thomas Greiner 2015/11/27 19:10:13 `reason[1]` doesn't appear to include any HTML so `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. saroyanm 2015/11/30 10:27:05 The reason why I used span, is that I wanted the t Show quoted text 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 "'". Thomas Greiner 2015/11/30 17:09:07 That appears a bit odd given that the style guide Show quoted text 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. Show quoted text > 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. saroyanm 2015/11/30 18:04:43 I guess we initially implemented something similar Show quoted text 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... Thomas Greiner 2015/12/02 16:41:04 Personally, I'd prefer avoid using `innerHTML` and Show quoted text 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). Thomas Greiner 2015/12/02 16:41:04 In case you missed it from the comment above… On In case you missed it from the comment above… On 2015/11/30 10:27:05, saroyanm wrote: Show quoted text > 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. saroyanm 2015/12/02 18:47:58 Done. Show quoted text 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.
	48 labelElement.appendChild(checkboxElement);

	49 labelElement.appendChild(spanElement);

	50 listElement.appendChild(labelElement);

	51 var reasonContainer = document.getElementById("reasons");

	52 reasonContainer.insertBefore(listElement, reasonContainer.firstChild);

	53 }

	54

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

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

	57 {

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

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

	60 textArea.removeAttribute("class")

	61 else

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

	63 }, false);

	64

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

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

	67 {

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

	69 {

	70 event.preventDefault();

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

	72 }

	73 else

	74 {

	75 form.submit();

	76 }

	77 }, false);

	78 }

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

	80 })();

	81 </script>

	82 </head>

	83

	84 <section markdown="1" class="highlighted">

	85 #{{reasons-header[Form heading] Please select the reason(s) why you uninstalled Adblock Plus:}}

	86

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

	88 <ul id="reasons">

	89 <li>

	90 <label>

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

	92 <span>{{reason-other[Last uninstallation reason] Other, namely...}}</spa n>

	93 </label>

	94 <textarea id="reason-other-input" class="hidden" name="reasonOther" placeh older="{{reason-other-placeholder[Textarea placeholder text, appears after selec ting 'Other, namely...' option] Please explain why you are uninstalling Adblock Plus}}"></textarea>

	95 </li>

	96 </ul>

	97 <button id="submit-form" type="button">{{submit[Submit button text] Submit}}</ button>
	Thomas Greiner 2015/11/27 19:10:12 Detail: It's a `<button>` element so isn't the `ty Detail: It's a `<button>` element so isn't the `type="button"` attribute redundant? saroyanm 2015/11/30 10:27:06 Done. Show quoted text 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.
	98 <span class="error-label">{{error-msg[Error message, is being shown after subm ission if no item selected] Please select at least one of the options above}}</s pan>

	99 </form>

	100

	101 {{disclaimer[Disclaimer below form] By clicking Submit, you are sending your res ponse to Adblock Plus. Please see our [privacy policy](https://adblockplus.org/p rivacy).}}
	Thomas Greiner 2015/11/27 19:10:12 Detail: This page is located on https://adblockplu Detail: This page is located on https://adblockplus.org so we should be able to simply link to "privacy" instead. saroyanm 2015/11/30 10:27:06 Done. Show quoted text 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.
	102 {: .disclaimer }

	103 </section>

OLD	NEW

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