Issue 29348067: Issue 4049 - reinstall button and adbockers list added to uninstallation page (Closed)
|
Created:
July 20, 2016, 2:04 p.m. by saroyanm Modified:
Sept. 30, 2016, 11:37 a.m. Reviewers:
juliandoucette Visibility:
Public. |
DescriptionIssue 4049 - reinstall button and adbockers list added to uninstallation page
Patch Set 1 : #Patch Set 2 : Fixed some syntax issues #
Total comments: 23
Patch Set 3 : Addressed Julian's comments #
Total comments: 14
Patch Set 4 : And again addressed comments #MessagesTotal messages: 7
@Julian can you please have a look when you have time.
https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:61: document.getElementById(element).classList.remove("hidden"); classList is not supported < IE11 https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:97: <section class="clear"> I suggest changing this <strong> to a heading or this <section> to a <div>. "The HTML <section> element represents a generic section of a document, i.e., a thematic grouping of content, typically with a heading. Each <section> should be identified, typically by including a heading (<h1>-<h6> element) as a child of the <section> element." - https://developer.mozilla.org/en/docs/Web/HTML/Element/section https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:112: <ul class="hidden" id="adblockers"> Why use a <ul> here? Perhaps a fieldset would be more appropriate? https://developer.mozilla.org/en/docs/Web/HTML/Element/fieldset https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:114: <label> Why use a label if there is no label text? https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:117: <option name="otherAdblockerPredefined" hide-element="other-adblocker" value="{{name}}">{{name}}</option> Invalid attribute "hide-element". See https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:119: <option name="otherAdblockerPredefined" show-element="other-adblocker" value="Other">{{"Other"|translate("other", "Option in list of Adblockers")}}</option> Invalid attribute "show-element". See https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:130: <input id="reason-other" toggle-view="reason-other-container" type="checkbox" name="reason" value="0v0" /> Invalid attribute "toggle-view". See https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:133: <ul id="reason-other-container" class="hidden"> Why use a ul here? Perhaps a fieldset would be more appropriate? https://developer.mozilla.org/en/docs/Web/HTML/Element/fieldset https://codereview.adblockplus.org/29348067/diff/29348080/static/css/simple.css File static/css/simple.css (right): https://codereview.adblockplus.org/29348067/diff/29348080/static/css/simple.c... static/css/simple.css:105: { - why section.clear and not .clear? - it seems like section.clear and section.clear a styles are page specific - shouldn't they go in the page template? - shouldn't they be more descriptive? https://codereview.adblockplus.org/29348067/diff/29348080/static/css/simple.c... static/css/simple.css:215: align-items: flex-start; This is not supported < IE11
https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:133: <ul id="reason-other-container" class="hidden"> See https://www.w3.org/TR/html5/forms.html#the-fieldset-element for better examples.
https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:61: document.getElementById(element).classList.remove("hidden"); On 2016/07/28 23:05:41, juliandoucette wrote: > classList is not supported < IE11 Uninstallation page is only being shown to the Chrome users. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:97: <section class="clear"> On 2016/07/28 23:05:42, juliandoucette wrote: > I suggest changing this <strong> to a heading or this <section> to a <div>. > > "The HTML <section> element represents a generic section of a document, i.e., a > thematic grouping of content, typically with a heading. Each <section> should be > identified, typically by including a heading (<h1>-<h6> element) as a child of > the <section> element." > - https://developer.mozilla.org/en/docs/Web/HTML/Element/section Fare enough, done. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:112: <ul class="hidden" id="adblockers"> On 2016/07/28 23:05:41, juliandoucette wrote: > Why use a <ul> here? > > Perhaps a fieldset would be more appropriate? > https://developer.mozilla.org/en/docs/Web/HTML/Element/fieldset Done. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:114: <label> On 2016/07/28 23:05:41, juliandoucette wrote: > Why use a label if there is no label text? Done. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:117: <option name="otherAdblockerPredefined" hide-element="other-adblocker" value="{{name}}">{{name}}</option> On 2016/07/28 23:05:41, juliandoucette wrote: > Invalid attribute "hide-element". See > https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes. Done. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:119: <option name="otherAdblockerPredefined" show-element="other-adblocker" value="Other">{{"Other"|translate("other", "Option in list of Adblockers")}}</option> On 2016/07/28 23:05:41, juliandoucette wrote: > Invalid attribute "show-element". See > https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes. Done. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:130: <input id="reason-other" toggle-view="reason-other-container" type="checkbox" name="reason" value="0v0" /> On 2016/07/28 23:05:42, juliandoucette wrote: > Invalid attribute "toggle-view". See > https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes. Done. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:133: <ul id="reason-other-container" class="hidden"> On 2016/07/28 23:05:41, juliandoucette wrote: > Why use a ul here? > > Perhaps a fieldset would be more appropriate? > https://developer.mozilla.org/en/docs/Web/HTML/Element/fieldset I kinda was thinking that fieldset should require legend tag inside, but apparently it not, can't say anything against using it, Done. https://codereview.adblockplus.org/29348067/diff/29348080/static/css/simple.css File static/css/simple.css (right): https://codereview.adblockplus.org/29348067/diff/29348080/static/css/simple.c... static/css/simple.css:105: { On 2016/07/28 23:05:42, juliandoucette wrote: > - why section.clear and not .clear? The implementation currently is changed, I think this behaves as a notification above sections. > - it seems like section.clear and section.clear a styles are page specific > - shouldn't they go in the page template? If more pages will have this kind of block "Notification" in that case no, but not sure on current state, so I think we can have it here for now. > - shouldn't they be more descriptive? Yes they should, hope currently it's more descriptive, let me know if you think it's not. https://codereview.adblockplus.org/29348067/diff/29348080/static/css/simple.c... static/css/simple.css:215: align-items: flex-start; On 2016/07/28 23:05:42, juliandoucette wrote: > This is not supported < IE11 Hmm not sure why this was here in first place:/ I think we don't need it here at all.
Generally looks good. A couple really minor issues. Sorry about the delay. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:61: document.getElementById(element).classList.remove("hidden"); On 2016/08/24 11:48:41, saroyanm wrote: > On 2016/07/28 23:05:41, juliandoucette wrote: > > classList is not supported < IE11 > > Uninstallation page is only being shown to the Chrome users. Acknowledged. https://codereview.adblockplus.org/29348067/diff/29348080/pages/uninstalled.t... pages/uninstalled.tmpl:97: <section class="clear"> On 2016/08/24 11:48:42, saroyanm wrote: > On 2016/07/28 23:05:42, juliandoucette wrote: > > I suggest changing this <strong> to a heading or this <section> to a <div>. > > > > "The HTML <section> element represents a generic section of a document, i.e., > a > > thematic grouping of content, typically with a heading. Each <section> should > be > > identified, typically by including a heading (<h1>-<h6> element) as a child of > > the <section> element." > > - https://developer.mozilla.org/en/docs/Web/HTML/Element/section > > Fare enough, done. I should have suggested changing this section to an aside and this strong to a heading. My mistake. (I don't think this matters very much because this page is not intended for the website and search engines.) https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:48: reasonsContainer.addEventListener("click", function(event) NIT: I would suggest listening to the "change" event instead of the "click" event on checkboxes / radiobuttons. 1. I think it's *more* correct 2. The handler won't be fired on the label when the label is clicked https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:52: document.getElementById(toggleView).classList.toggle("hidden"); Minor bug: If you check the "Other, namely..." checkbox and then refresh the page than: 1. The textarea disappears 2. The checkbox (show/hide)s backwards To correct this, you could: 1. Validate that the checkbox is checked (`event.target.checked`) 2. Run this code on load and when the checkbox is checked https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:129: <textarea id="reason-other-input" name="reasonOther" maxlength="300" placeholder="{{"Please explain why you are uninstalling Adblock Plus"|translate("reason-other-placeholder", "Textarea placeholder text, appears after selecting 'Other, namely...' option")}}"></textarea> This textarea is pretty small (it fits about 100 char). Have we considered setting rows or height? https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.css File static/css/simple.css (right): https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.c... static/css/simple.css:109: section, .notification NIT: Shouldn't this be on the next line? https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.c... static/css/simple.css:193: button, .notification a NIT: Shouldn't this be on the next line? https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.c... static/css/simple.css:201: section.highlighted form button NIT: We don't need "section" here?
https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:48: reasonsContainer.addEventListener("click", function(event) On 2016/09/24 16:49:33, juliandoucette wrote: > NIT: > > I would suggest listening to the "change" event instead of the "click" event on > checkboxes / radiobuttons. I agree that in this case we can go with the change event. Done. > 1. I think it's *more* correct I think it's more a design question, whether your intention are to assign event listener on each element, or only to one, or even to body, but in case of small amount of elements we should be fine, considering memory efficient solution. > 2. The handler won't be fired on the label when the label is clicked It depends, if you will have label containing the input element it will be fired twice, one for the label another for the input: http://stackoverflow.com/questions/17185265/jquery-click-event-triggers-twice... https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:52: document.getElementById(toggleView).classList.toggle("hidden"); On 2016/09/24 16:49:33, juliandoucette wrote: > Minor bug: > > If you check the "Other, namely..." checkbox and then refresh the page than: > > 1. The textarea disappears > 2. The checkbox (show/hide)s backwards > > To correct this, you could: > > 1. Validate that the checkbox is checked (`event.target.checked`) > 2. Run this code on load and when the checkbox is checked Good catch, Chrome wasn't refreshing the selected items, totally forgot about that. https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:129: <textarea id="reason-other-input" name="reasonOther" maxlength="300" placeholder="{{"Please explain why you are uninstalling Adblock Plus"|translate("reason-other-placeholder", "Textarea placeholder text, appears after selecting 'Other, namely...' option")}}"></textarea> On 2016/09/24 16:49:34, juliandoucette wrote: > This textarea is pretty small (it fits about 100 char). Have we considered > setting rows or height? It has height and width specified, AFAIK it was requested with that sizes, at least it was aligned with Designers as far as I remember, but I'm open to change it size as well, I'll suggest to that kind of stylistic change separately. https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.css File static/css/simple.css (right): https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.c... static/css/simple.css:109: section, .notification On 2016/09/24 16:49:34, juliandoucette wrote: > NIT: > > Shouldn't this be on the next line? Done. https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.c... static/css/simple.css:193: button, .notification a On 2016/09/24 16:49:34, juliandoucette wrote: > NIT: > > Shouldn't this be on the next line? Done. https://codereview.adblockplus.org/29348067/diff/29350166/static/css/simple.c... static/css/simple.css:201: section.highlighted form button On 2016/09/24 16:49:34, juliandoucette wrote: > NIT: > > We don't need "section" here? Done.
LGTM https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.tmpl File pages/uninstalled.tmpl (right): https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:48: reasonsContainer.addEventListener("click", function(event) On 2016/09/27 12:10:14, saroyanm wrote: > On 2016/09/24 16:49:33, juliandoucette wrote: > > NIT: > > > > I would suggest listening to the "change" event instead of the "click" event > on > > checkboxes / radiobuttons. > I agree that in this case we can go with the change event. Done. > > 1. I think it's *more* correct > I think it's more a design question, whether your intention are to assign event > listener on each element, or only to one, or even to body, but in case of small > amount of elements we should be fine, considering memory efficient solution. > > 2. The handler won't be fired on the label when the label is clicked > It depends, if you will have label containing the input element it will be fired > twice, one for the label another for the input: > http://stackoverflow.com/questions/17185265/jquery-click-event-triggers-twice... Acknowledged. https://codereview.adblockplus.org/29348067/diff/29350166/pages/uninstalled.t... pages/uninstalled.tmpl:129: <textarea id="reason-other-input" name="reasonOther" maxlength="300" placeholder="{{"Please explain why you are uninstalling Adblock Plus"|translate("reason-other-placeholder", "Textarea placeholder text, appears after selecting 'Other, namely...' option")}}"></textarea> On 2016/09/27 12:10:14, saroyanm wrote: > On 2016/09/24 16:49:34, juliandoucette wrote: > > This textarea is pretty small (it fits about 100 char). Have we considered > > setting rows or height? > > It has height and width specified, AFAIK it was requested with that sizes, at > least it was aligned with Designers as far as I remember, but I'm open to change > it size as well, I'll suggest to that kind of stylistic change separately. Acknowledged. 
|
