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: Created Nov. 11, 2015, 5: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

	16 <script type="text/javascript">

	17 (function()

	18 {

	19 function init()

	20 {

	21 window.location.search.substr(1).split("&").forEach(function(param)
	Thomas Greiner 2015/11/11 19:08:29 According to the ticket description the page is su 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. saroyanm 2015/11/12 15:50:24 Yes you right, I should have left a note regarding Show quoted text 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.
	22 {

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

	24 var paramName = paramSplit[0];

	25 var paramValue = paramSplit[1];

	26

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

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

	29 if (paramName)
	Thomas Greiner 2015/11/11 19:08:28 What do you want to check for? If there's no name 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; saroyanm 2015/11/12 15:50:24 Fare enough. Done. Show quoted text 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.
	30 input.setAttribute("name", paramName);

	31 if (paramValue)

	32 input.setAttribute("value", paramValue);

	33

	34 document.getElementById("paramsWrapper").appendChild(input);
	Thomas Greiner 2015/11/11 19:08:28 Since those input fields are hidden anyway, it doe 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. saroyanm 2015/11/12 15:50:24 Done. Show quoted text 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.
	35 });

	36

	37 var form = document.getElementsByTagName("form")[0];

	38 document.getElementById("reasonOther").addEventListener("change",

	39 function()

	40 {

	41 var textArea = form.getElementsByTagName("textarea")[0];

	42 textArea.hasAttribute("class") ? textArea.removeAttribute("class") :

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

	44 }, false);

	45

	46 form.getElementsByTagName("button")[0].addEventListener("click",
	Thomas Greiner 2015/11/11 19:08:28 In this case I'd recommend using `getElementsById` In this case I'd recommend using `getElementsById` to avoid issues in case more buttons get added to the form (same with "textarea" above). saroyanm 2015/11/12 15:50:24 Done. Show quoted text 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.
	47 function(event)

	48 {

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

	50 {

	51 event.preventDefault();

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

	53 }

	54 }, false);

	55 }

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

	57 })();

	58 </script>

	59 </head>

	60 <section class="highlighted">

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

	62 <form action="uninstall-abp-submit" method="get">
	Thomas Greiner 2015/11/11 19:08:28 According to the ticket description the form shoul According to the ticket description the form should be submitted via POST. saroyanm 2015/11/12 15:50:24 Ohh wow, thanks. Show quoted text 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.
	63 <ul>

	64 {%- for reasonId, stringId, value in reasons\|shuffle %}

	65 <li>

	66 <label>

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

	68 <span>{{value\|translate(stringId, "One of the listed uninstallation re ason")}}</span>

	69 </label>

	70 </li>

	71 {%- endfor %}

	72 <li>

	73 <label>

	74 <input id="reasonOther" type="checkbox" name="reason" value="0v0" />
	Thomas Greiner 2015/11/11 19:08:28 Detail: This ID value is not corresponding to our Detail: This ID value is not corresponding to our coding style (see https://google.github.io/styleguide/htmlcssguide.xml?showone=ID_and_Class_Nam...). saroyanm 2015/11/12 15:50:25 Ahh right! Done. Show quoted text 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.
	75 <span>{{"Other, namely..."\|translate("reason-other", "Last uninstallat ion reason")}}</span>

	76 </label>

	77 <textarea class="hidden" name="reasonOther" placeholder="{{"Please expla in why you are uninstalling Adblock Plus"\|translate("reason-other-placeholder", "Textarea placeholder text, appears after selecting 'Other, namely...' option")} }"></textarea>

	78 </li>

	79 </ul>

	80 <div id="paramsWrapper"></div>

	81 <button type="submit">{{"Submit"\|translate("submit", "Submition button text" )}}</button>
	Thomas Greiner 2015/11/11 19:08:28 Typo: Replace "Submition button text" with "Submit Typo: Replace "Submition button text" with "Submit button text" Thomas Greiner 2015/11/11 19:08:29 Note that this form will work even for visitors th 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. saroyanm 2015/11/12 15:50:24 I think if we can disable form submission for peop Show quoted text 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). saroyanm 2015/11/12 15:50:25 Done. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > Typo: Replace "Submition button text" with "Submit button text" Done. Thomas Greiner 2015/11/27 19:10:10 I suppose server-side generation of the form is of Show quoted text 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?
	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>
	Thomas Greiner 2015/11/11 19:08:29 Detail: Shortening "message" to "msg" here makes t 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. saroyanm 2015/11/12 15:50:24 Done. Show quoted text 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.
	83 </form>

	84 <p class="disclaimer">{{"By clicking Submit, you are sending your response to Adblock Plus. Please see our <a href='https://adblockplus.org/privacy'>privacy p olicy</a>."\|translate("disclaimer", "Disclaimer below form")}}</p>

	85 </section>

OLD	NEW

« no previous file with comments | « filters/shuffle.py ('k') | pages/uninstall-abp-submit.tmpl » ('j') | static/css/simple.css » ('J')