Issue 29565721: No Issue - Implemented first run page
|
Created:
Oct. 5, 2017, 1:47 p.m. by martin Modified:
Feb. 16, 2018, 4:19 p.m. CC:
juliandoucette Base URL:
https://hg.adblockplus.org/adblockplusui/ Visibility:
Public. |
DescriptionNo Issue - Implemented first run page
Patch Set 1 #
Total comments: 36
Patch Set 2 : Included Header / Footer, addressed previous feedback #
Total comments: 25
Patch Set 3 : Addressed newest round of feedback #
Total comments: 51
Patch Set 4 : Addressed newest round of feedback (2) #
Total comments: 12
Patch Set 5 : Addressed latest round of feedback #
Total comments: 31
MessagesTotal messages: 30
Here are my first impressions. Also: - Please updating coding-style according to https://adblockplus.org/coding-style (you may also be interested in our stylelintrc https://codereview.adblockplus.org/29541680/) - Please optimize images and svgs https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:33: <body> NIT: You should wrap the content currently on this page with <main> (See https://www.w3.org/TR/html5/grouping-content.html#the-main-element for explanation) https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:36: <img src="./skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark icon"> NIT: Alt text should be translated (The same applies elsewhere) https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:36: <img src="./skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark icon"> NIT: "./" is unnecessary here (The same applies elsewhere) https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:45: <div class="column one-third"> NIT: These (columns) should be <article>s (See https://www.w3.org/TR/html5/sections.html#the-article-element for explanation) (<section> vs <article> is debatable here. I'm qualifying this as "an interactive widget or gadget, or any other independent item of content." because their contents are reusable and ~different-enough (to be "independent") from each other (and the outer section).) https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:47: <h2 class="i18n_firstRun_columnOneTitle"></h2> NIT: I think that you should wrap the <img> and the <h2> in a <header> to communicate that the image is part of the heading. https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:57: <a href="" class="store-button applestore-button"></a> NIT: There is no text in this button You could use [a.title, a > img.alt, a > span.sr-only] (if you know what I mean?) (The same applies elsewhere.) https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:23: src: local ("Ø"), NIT: What the? https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:48: font-size: 16px; NIT: I prefer to set document wide styles on the root (html) element. (Acknowledging that it doesn't make a difference to this CSS e.g. because you are not using rem.) https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:49: background-color: white; NIT/Reminder: "CSS color values should be specified in hexadecimal where possible." (also, "#fff" is fewwer characters than "white" :P) https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:61: header { NOTE: Padding inside the .content body causes this to be offcenter ~400px https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:74: @media (min-width: 992px) { NIT: This doesn't make sense. - You are setting a width larger than the screen at 992px - Which doesn't take effect until > 1194px because the media query below is more specific - Where did you get 1194 from anyway? 1170 + 2em (at 16px) is 1202px? - This causes the layout to be offcenter at 1200px width https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:89: width: 100%; NIT: This kills the padding left/right because html is not box-sizing: border-box. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:93: .button-primary { NIT/Suggest: Make this button full-width on small mobile devices https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:97: color: white !important; NIT: This important is unnecessary? https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:101: height: 54px; NIT: px [height, width, line-height] kinda defeats the purpose of using em font-size (acknowledging that this is a very edge cases). You can use em here (and optionally use margin and padding to vertically centre). https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:108: width: 50%; NIT: This gets pretty awkward around 336px. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:139: display: inline-block; NIT: These buttons are not horizontally aligned on small screens (e.g. 320px) because of inline-block + whitespace. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:150: h1 { NIT: This doesn't fit on the screen ~320 Firefox https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:161: font-size: 1.25em; NIT: I see that you followed Paul's stytleguide ~exactly... I would prefer that all flow content inherited font-size from html (except for heading content). https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:162: font-weight: 300; NIT: We agreed (privately - sorry) to use 400 instead of 300 for websites. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:169: font-size: 1em; NIT: This doesn't actually do anything because em is relative. p of 1.25em > a of 1em = 1.25em. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:178: * Grid component Now that I think about it, you could include all of website-defaults main.css (suggest "defaults.css") instead of inserting this component into this CSS file. [the performance impact should be negligible, it should make updating easier, it should reduce maintenance cost e.g. if you change the grid component like you did with .column box-sizing below (which is normally applied by website-default base styles)]. I doubt that we will introduce npm & scss into adblockplusui anytime soon :(
Here's a couple of your comments addressed. I think I largely have the HTML where it should be. The points you made were very helpful. I have some questions about the CSS stuff and will be working on that next. As soon as I'm confident I resubmit the review. Cheers! https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:33: <body> On 2017/10/08 13:18:10, juliandoucette wrote: > NIT: You should wrap the content currently on this page with <main> > > (See https://www.w3.org/TR/html5/grouping-content.html#the-main-element for > explanation) Done. https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:36: <img src="./skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark icon"> On 2017/10/08 13:18:10, juliandoucette wrote: > NIT: Alt text should be translated > > (The same applies elsewhere) I honestly don't know how to do that with the current translation setup. If we were using some sort of templating I could have gone for something like alt={{ %translation-string }}, however I'm not sure how to translate alt tags currently :/ https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:45: <div class="column one-third"> On 2017/10/08 13:18:10, juliandoucette wrote: > NIT: These (columns) should be <article>s > > (See https://www.w3.org/TR/html5/sections.html#the-article-element for > explanation) > > (<section> vs <article> is debatable here. I'm qualifying this as "an > interactive widget or gadget, or any other independent item of content." because > their contents are reusable and ~different-enough (to be "independent") from > each other (and the outer section).) Done. https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:47: <h2 class="i18n_firstRun_columnOneTitle"></h2> On 2017/10/08 13:18:10, juliandoucette wrote: > NIT: I think that you should wrap the <img> and the <h2> in a <header> to > communicate that the image is part of the heading. Done. https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:57: <a href="" class="store-button applestore-button"></a> On 2017/10/08 13:18:10, juliandoucette wrote: > NIT: There is no text in this button > > You could use [a.title, a > img.alt, a > span.sr-only] (if you know what I > mean?) > > (The same applies elsewhere.) Soo... I *think* what you mean is that instead of setting a "background-image" to an empty <a>, I should nest an <img> inside and set the src of that? Or? https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:23: src: local ("Ø"), On 2017/10/08 13:18:12, juliandoucette wrote: > NIT: What the? My reaction exactly. It was like this in the original source, so I decided to keep it that way. Let me know what it does. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:49: background-color: white; On 2017/10/08 13:18:11, juliandoucette wrote: > NIT/Reminder: "CSS color values should be specified in hexadecimal where > possible." (also, "#fff" is fewwer characters than "white" :P) Done.
https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:57: <a href="" class="store-button applestore-button"></a> On 2017/10/16 09:09:09, martin wrote: > On 2017/10/08 13:18:10, juliandoucette wrote: > > NIT: There is no text in this button > > > > You could use [a.title, a > img.alt, a > span.sr-only] (if you know what I > > mean?) > > > > (The same applies elsewhere.) > > Soo... I *think* what you mean is that instead of setting a "background-image" > to an empty <a>, I should nest an <img> inside and set the src of that? Or? ~Yes. I suggested the following solutions: 1. Place a title attribute on the <a> 2. Place an <img> with an alt attribute in the <a> 3. Place a <span> that is screenreader-only in the <a> Of which, solution #2 is the most correct. (An even better solution would be to create the entire button using HTML/CSS and an image icon with alt text.)
I reviewed only comment Julian asked me, but in general I think Module owner or peer should be involved in the changes to the repository. I'll have final review after all comments addressed. Also I can see comments that states "Done", but I can't see new patch with the change https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:36: <img src="./skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark icon"> On 2017/10/16 09:09:08, martin wrote: > On 2017/10/08 13:18:10, juliandoucette wrote: > > NIT: Alt text should be translated > > > > (The same applies elsewhere) > > I honestly don't know how to do that with the current translation setup. If we > were using some sort of templating I could have gone for something like alt={{ > %translation-string }}, however I'm not sure how to translate alt tags currently > :/ text inside of attributes can't be easily translated, it fact it's not even translated for new options page. I'll suggest to keep the way it is right now and handle that in separate issue, not to over-complicate this review. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:23: src: local ("Ø"), On 2017/10/16 09:09:09, martin wrote: > On 2017/10/08 13:18:12, juliandoucette wrote: > > NIT: What the? > > My reaction exactly. It was like this in the original source, so I decided to > keep it that way. Let me know what it does. It's used to force not using local font, I think the previous comments applies here as well. https://www.paulirish.com/2009/bulletproof-font-face-implementation-syntax/#s...
Please also include an issue, or at least relevant links to the review.
Added Thomas as reviewer.
https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:36: <img src="./skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark icon"> On 2017/10/16 09:09:08, martin wrote: > On 2017/10/08 13:18:10, juliandoucette wrote: > > NIT: Alt text should be translated > > > > (The same applies elsewhere) > > I honestly don't know how to do that with the current translation setup. If we > were using some sort of templating I could have gone for something like alt={{ > %translation-string }}, however I'm not sure how to translate alt tags currently > :/ Done. https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newco... firstRun.html:57: <a href="" class="store-button applestore-button"></a> On 2017/10/16 10:00:38, juliandoucette wrote: > On 2017/10/16 09:09:09, martin wrote: > > On 2017/10/08 13:18:10, juliandoucette wrote: > > > NIT: There is no text in this button > > > > > > You could use [a.title, a > img.alt, a > span.sr-only] (if you know what I > > > mean?) > > > > > > (The same applies elsewhere.) > > > > Soo... I *think* what you mean is that instead of setting a "background-image" > > to an empty <a>, I should nest an <img> inside and set the src of that? Or? > > ~Yes. > > I suggested the following solutions: > > 1. Place a title attribute on the <a> > 2. Place an <img> with an alt attribute in the <a> > 3. Place a <span> that is screenreader-only in the <a> > > Of which, solution #2 is the most correct. > > (An even better solution would be to create the entire button using HTML/CSS and > an image icon with alt text.) Done. I went for option 2. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:48: font-size: 16px; On 2017/10/08 13:18:11, juliandoucette wrote: > NIT: I prefer to set document wide styles on the root (html) element. > (Acknowledging that it doesn't make a difference to this CSS e.g. because you > are not using rem.) Done. https://codereview.adblockplus.org/29565721/diff/29565722/skin/firstRun.css#n... skin/firstRun.css:101: height: 54px; On 2017/10/08 13:18:12, juliandoucette wrote: > NIT: px [height, width, line-height] kinda defeats the purpose of using em > font-size (acknowledging that this is a very edge cases). You can use em here > (and optionally use margin and padding to vertically centre). Done.
The latest PatchSet does not apply. Can you please updated and rebase?
https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html#newco... firstRun.html:49: <a href="/en/about" hreflang="en">About</a> Missing translations (the same applies elsewhere) https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html#newco... firstRun.html:65: <a href="#" id="navbar-locale-selected"> This dropdown is not working. https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html#newco... firstRun.html:71: <a href="/ar/firefox" hreflang="ar"> Wrong href (the same applies elsewhere) https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html#newco... firstRun.html:173: <header class="installation-status-header"> NIT: This class is too specific and not re-used. I suggest an ID. https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html#newco... firstRun.html:228: <li><a href="/en/acceptable-ads" hreflang="en">Acceptable Ads</a></li> href should have https://adblockplus.org/... (the same applies elsewhere) https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:52: font-size: 16px; This is unnecessary (because the default is 16px) and not-accessible (because it overrides the browser font-size). https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:57: background-color: #fff; This is unnecessary (because the default is white). https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:70: .column-centered-content SuperNIT: We usually call this `ta-centre` https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:77: .one-third NIT: This selector is too general (you mean all one-third columns in the body, not *all* one-third columns). https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:85: * .content SuperNIT: You removed the space before the "*" (the same applies elsewhere) (SuperNIT = Less than NIT) https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:99: .content h1 { font-size: 2.4em; } Note: I have added smaller heading sizes for mobile recently https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:375: There should be some sort of separation here so that we don't confuse these with #navbar styles. https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:376: .installation-status-header See comment in firstRun.html https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:427: } Missing right-to-left style [dir="rtl"] .appstore-button { margin-right: 0; margin-left: 1em; } https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:446: h1 NIT: I think that this selector is too general (I think that you mean all h1 inside of .installation-status-header) https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:452: p I think that all of these styles are already covered correctly by defaults.css. https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:459: .column-description > a I think that this selector is too specific. I think that you want all links in the main content to be red. e.g. .content a, .content a:visited { color: #c70d2c; } I think that font-weight and text-decoration are already covered by defaults.css https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:466: p > a:hover I think that this is already covered by defaults.css
https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:77: .one-third On 2017/10/18 10:42:37, juliandoucette wrote: > NIT: This selector is too general (you mean all one-third columns in the body, > not *all* one-third columns). How about I give the parent div a class of "content-columns-container" and then do `.content-columns-container > .one-third`? https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:99: .content h1 { font-size: 2.4em; } On 2017/10/18 10:42:37, juliandoucette wrote: > Note: I have added smaller heading sizes for mobile recently I pulled the latest changes for `website-defaults`, rebuilt by running `gulp`, looked through `main.css` but couldn't find any media queries associated with heading sizes. Where should I find these? https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:459: .column-description > a I went for this specific of a selector because when I did `.content > a` all links remained blue... On 2017/10/18 10:42:37, juliandoucette wrote: > I think that this selector is too specific. I think that you want all links in > the main content to be red. e.g. > > .content a, > .content a:visited > { > color: #c70d2c; > } > > I think that font-weight and text-decoration are already covered by defaults.css
DFTBA https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:77: .one-third On 2017/10/20 10:03:28, martin wrote: > On 2017/10/18 10:42:37, juliandoucette wrote: > > NIT: This selector is too general (you mean all one-third columns in the body, > > not *all* one-third columns). > > How about I give the parent div a class of "content-columns-container" and then > do `.content-columns-container > .one-third`? By "body" I didn't mean <body> or `body .one-third`. I meant `#content .one-third` or similar. You have the right idea. But classes are meant to be reused. Use an ID unless this rule applies to more than one page that you know about. https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:99: .content h1 { font-size: 2.4em; } On 2017/10/20 10:03:27, martin wrote: > On 2017/10/18 10:42:37, juliandoucette wrote: > > Note: I have added smaller heading sizes for mobile recently > > I pulled the latest changes for `website-defaults`, rebuilt by running `gulp`, > looked through `main.css` but couldn't find any media queries associated with > heading sizes. Where should I find these? Ack. I didn't push this (https://codereview.adblockplus.org/29581842/) yes. Sorry. https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:459: .column-description > a On 2017/10/20 10:03:27, martin wrote: > I went for this specific of a selector because when I did `.content > a` all > links remained blue... That's because [the child combinator only matches one level deep, two selectors are more specific than one]. If you use `.content a, .content a:visited` and place this CSS file after defaults.css then it will take prescience. @see https://developer.mozilla.org/en-US/docs/Web/CSS/Child_selectors PS: In the future we may integrate SCSS into adblockplusui (hopefully) and use the link color variable when generating defaults.css instead. Sorry about this :/
Sorry for confusion :( https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29581817/skin/firstRun.css#n... skin/firstRun.css:99: .content h1 { font-size: 2.4em; } On 2017/10/20 13:50:21, juliandoucette wrote: > On 2017/10/20 10:03:27, martin wrote: > > On 2017/10/18 10:42:37, juliandoucette wrote: > > > Note: I have added smaller heading sizes for mobile recently > > > > I pulled the latest changes for `website-defaults`, rebuilt by running `gulp`, > > looked through `main.css` but couldn't find any media queries associated with > > heading sizes. Where should I find these? > > Ack. I didn't push this (https://codereview.adblockplus.org/29581842/) yes. > Sorry. Not to mention that this isn't part of website-defaults. I added smaller mobile sizes to new abp.org main.css. You can copy & paste them from here. .content h1 { font-size: 1.8em; } .content h2 { font-size: 1.4em; } .content h3 { font-size: 1.3em; } .content h4 { font-size: 1.2em; } .content h5 { font-size: 1.1em; } @media(min-width: 768px) { .content h1 { font-size: 2.4em; } .content h2 { font-size: 1.6em; } } PS: We can probably bundle main.css from abp.org into abpui in the future instead of maintaining his CSS in firstRun.css.
Hey all! I addressed most of the last round's feedback. I (hopefully) managed to submit a working patch this time. If not - do let me know so I can fix it.
On 2017/10/20 14:46:36, martin wrote: > Hey all! > > I addressed most of the last round's feedback. I (hopefully) managed to submit a > working patch this time. If not - do let me know so I can fix it. I'm guessing that you didn't mean to delete the issue reporter?
@greiner, @saroyanm please find questions below. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:71: <a href="https://adblockplus.org/ar/firefox" hreflang="ar"> These hrefs are not correct. We want to translate this page, not goto adblockplus.org. The last first run page didn't have a language selector. Perhaps @greiner can explain how this page is / can be translated? https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:237: <h5class="i18n_firstRun_footerCommunityHeader"></h5> Missing space. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode18 firstRun.js:18: /* globals checkShareResource, getDocLink, openSharePopup, setLinks, E */ @greiner, @saroyanm "[Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/." Is this something that we should worry about now? https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:2: * This file is part of Adblock Plus <https://adblockplus.org/>, Please keep the spaces before "*" in these comments https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:18: @font-face This doesn't seem to be used anywhere? https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:41: font-style: normal; NIT: You could name this "bold" https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:62: width: 80%; new abp.org uses website-defaults .container width. Why are you setting a custom width for this page? https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:64: margin: 0 auto 8em; NIT: `.container` is a very general class. It's meant to centre the content within, and control the width and padding of, horizontal sections of the page. And it's currently being used in the main content *and* the footer. I'm guessing that you are trying to add 8em of space between the main content and the footer. If I am guessing correctly then I would suggest using a more specific selector for this purpose e.g. Adding an ID to the main content and applying an 8em margin-bottom to it. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:75: #content-columns-container > .one-third NIT: `#content-columns-container` is quite long. We currently use `#content` for this purpose on adblockplus.org. I suggest we use `#content` for the sake of consistency.
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:71: <a href="https://adblockplus.org/ar/firefox" hreflang="ar"> On 2017/10/21 16:18:42, juliandoucette wrote: > These hrefs are not correct. We want to translate this page, not goto > http://adblockplus.org. The last first run page didn't have a language selector. > Perhaps @greiner can explain how this page is / can be translated? IIRC I brought this up in a UI meeting that it doesn't make sense to have the language selector in the first-run page. Also I'd like to point out that we don't include any links in the extension directly but instead use documentation links (see spec) to ensure that links don't break. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode18 firstRun.js:18: /* globals checkShareResource, getDocLink, openSharePopup, setLinks, E */ On 2017/10/21 16:18:43, juliandoucette wrote: > @greiner, @saroyanm > > "[Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated > because of its detrimental effects to the end user's experience. For more help, > check https://xhr.spec.whatwg.org/.%22 > > Is this something that we should worry about now? No, this issue only applies to the development environment because we have to load translations synchronously to mock `chrome.i18n.getMessage()`. Obviously, we still need to change our mock implementation at some point to avoid breakage but it's not relevant for running the UI within the extension.
I quickly looked through the code and noticed a couple of things that caught my attention. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:40: <img src="skin/img/navbar-logo.png" srcset="skin/img/navbar-logo.svg 2x"> What's the reason for including both PNG as well as SVG files for each icon? https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:41: <span>Adblock <strong>Plus</strong></span> Please make sure that any text we show in the UI is translatable. That also includes alt-attributes unless they're only there for decoration. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:188: <p id="first-column-description" class="i18n_firstRun_columnOneDescription column-description"></p> Detail: The order of the columns may change. Generally, we don't include styling or layouting information in such IDs but describe the purpose of the text. Otherwise we may end up having to translate the same text again simply because of changes to its ID. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:199: <a href="" class="store-button applestore-button"> Don't we have the links for those buttons yet? https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:244: <li><a href="https://adblockplus.org/en/development-builds" class="i18n_firstRun_footerDevelopmentBuildsLink" hreflang="en"></a></li> Detail: Wrong indentation level and inconsisent spacing before and after. Same applies to other `<li>` elements in this file. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:271: <img src="skin/img/footer-twitter-glyphicon.png" alt="Twitter"> I assume that those icons are were not created by us. If that's the case, are there any license terms we have to consider when distributing them publicly as well as including them in an open source product? https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode49 firstRun.js:49: function toggleClass(element, className) We've already been using `Element.classList` in other pages so please use it instead of introducing those utility functions for handling CSS classes. https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... File locale/en_US/firstRun.json (right): https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... locale/en_US/firstRun.json:14: "firstRun_title": { Coding style: "No whitespace at the end of a line." https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... locale/en_US/firstRun.json:15: "message": "INSTALLATION SUCCESSFUL!" Detail: The capitalization of this text (and others in this file) is styling information which may change. Therefore we tend not to include the text as if it weren't in all caps (e.g. "Installation Successful") and apply the style information separately via CSS (i.e. `text-transform: uppercase`). https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... locale/en_US/firstRun.json:24: "message": "By default, you may see some nonintrisive ads that adhere to strict criteria. We identify these ads as Acceptable Ads. Prefer to block all ads? <a>Turn off Acceptable Ads</a> feature in seconds." Typo: Replace "nonintrinsive" with "nonintrusive". https://codereview.adblockplus.org/29565721/diff/29584656/skin/icons/first-ru... File skin/icons/first-run/icon-checkmark.svg (right): https://codereview.adblockplus.org/29565721/diff/29584656/skin/icons/first-ru... skin/icons/first-run/icon-checkmark.svg:1: <svg width="100" height="100" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg"><title>icon-checkmark</title><desc>Created with Sketch.</desc><g fill="none"><path d="M29.289 0l-29.289 29.29v41.421l29.289 29.289h41.423l29.288-29.289v-41.421l-29.288-29.29h-41.423zm-27.289 69.881v-39.763l28.119-28.118h39.763l28.118 28.118v39.763l-28.118 28.119h-39.763l-28.119-28.119z" fill="#C70D2C"/><path stroke="#C70D2C" stroke-width="2" d="M67.733 33l-25.455 19.831-12.075-6.679-6.202 4.85 18.168 17.998 33.832-32.443z"/></g></svg> Why did you include both PNG as well as SVG files for most icons? https://codereview.adblockplus.org/29565721/diff/29584656/skin/issue-reporter... File skin/issue-reporter.css (left): https://codereview.adblockplus.org/29565721/diff/29584656/skin/issue-reporter... skin/issue-reporter.css:1: body Be careful when merging your changes with master. Let's revert this change again (same applies to other unrelated files you removed).
Thanks Thomas. Please find follow-up questions below. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:40: <img src="skin/img/navbar-logo.png" srcset="skin/img/navbar-logo.svg 2x"> On 2017/10/23 13:17:42, Thomas Greiner wrote: > What's the reason for including both PNG as well as SVG files for each icon? He copied and pasted the new abp.org header. It's unclear to me how much we want to deviate from the abp.org implementation. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:41: <span>Adblock <strong>Plus</strong></span> On 2017/10/23 13:17:42, Thomas Greiner wrote: > Please make sure that any text we show in the UI is translatable. That also > includes alt-attributes unless they're only there for decoration. Is it necessary to make text that we do not translate translatable in ABPUI? (We don't translate "Adblock Plus" AFAIK.) https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:271: <img src="skin/img/footer-twitter-glyphicon.png" alt="Twitter"> On 2017/10/23 13:17:41, Thomas Greiner wrote: > I assume that those icons are were not created by us. If that's the case, are > there any license terms we have to consider when distributing them publicly as > well as including them in an open source product? Good question. I'll have to forward it along to product. I didn't ask :/ https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode49 firstRun.js:49: function toggleClass(element, className) On 2017/10/23 13:17:43, Thomas Greiner wrote: > We've already been using `Element.classList` in other pages so please use it > instead of introducing those utility functions for handling CSS classes. He copied this from abp.org main.js. It's unclear to me whether we should re-write this for abpui or just separate it. What do you think?
Thinking out loud. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode49 firstRun.js:49: function toggleClass(element, className) On 2017/10/23 13:27:35, juliandoucette wrote: > On 2017/10/23 13:17:43, Thomas Greiner wrote: > > We've already been using `Element.classList` in other pages so please use it > > instead of introducing those utility functions for handling CSS classes. > > He copied this from http://abp.org main.js. It's unclear to me whether we should > re-write this for abpui or just separate it. What do you think? (On that note, we could polyfill classList and refactor main.js in abp.org.)
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:40: <img src="skin/img/navbar-logo.png" srcset="skin/img/navbar-logo.svg 2x"> On 2017/10/23 13:27:35, juliandoucette wrote: > On 2017/10/23 13:17:42, Thomas Greiner wrote: > > What's the reason for including both PNG as well as SVG files for each icon? > > He copied and pasted the new http://abp.org header. It's unclear to me how much we want > to deviate from the http://abp.org implementation. As far as I know SVGs are served on high-dpi displays, hence the reason for including them. As @juliandoucette pointed out I simply copy / pasted his implementation of the header. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:41: <span>Adblock <strong>Plus</strong></span> On 2017/10/23 13:27:34, juliandoucette wrote: > On 2017/10/23 13:17:42, Thomas Greiner wrote: > > Please make sure that any text we show in the UI is translatable. That also > > includes alt-attributes unless they're only there for decoration. > > Is it necessary to make text that we do not translate translatable in ABPUI? (We > don't translate "Adblock Plus" AFAIK.) I intentionally didn't i18n this because it's more or less a brand name / logo. Not sure whether we should be translating it. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:71: <a href="https://adblockplus.org/ar/firefox" hreflang="ar"> On 2017/10/23 12:01:55, Thomas Greiner wrote: > On 2017/10/21 16:18:42, juliandoucette wrote: > > These hrefs are not correct. We want to translate this page, not goto > > http://adblockplus.org. The last first run page didn't have a language > selector. > > Perhaps @greiner can explain how this page is / can be translated? > > IIRC I brought this up in a UI meeting that it doesn't make sense to have the > language selector in the first-run page. > > Also I'd like to point out that we don't include any links in the extension > directly but instead use documentation links (see spec) to ensure that links > don't break. What I know for sure is that the spec I'm currently working off of, does indeed include a language selector. If we decide to not include it in this iteration we should probably ping Jeen so that she can update the spec? I'm not sure as to what the proper procedure should be. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:188: <p id="first-column-description" class="i18n_firstRun_columnOneDescription column-description"></p> On 2017/10/23 13:17:41, Thomas Greiner wrote: > Detail: The order of the columns may change. Generally, we don't include styling > or layouting information in such IDs but describe the purpose of the text. > Otherwise we may end up having to translate the same text again simply because > of changes to its ID. Completely agree. Will fix that. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:199: <a href="" class="store-button applestore-button"> On 2017/10/23 13:17:42, Thomas Greiner wrote: > Don't we have the links for those buttons yet? We probably do, however they are not included in the spec. Will try and clarify that with Jeen. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:237: <h5class="i18n_firstRun_footerCommunityHeader"></h5> On 2017/10/21 16:18:42, juliandoucette wrote: > Missing space. Will fix. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:244: <li><a href="https://adblockplus.org/en/development-builds" class="i18n_firstRun_footerDevelopmentBuildsLink" hreflang="en"></a></li> On 2017/10/23 13:17:42, Thomas Greiner wrote: > Detail: Wrong indentation level and inconsisent spacing before and after. Same > applies to other `<li>` elements in this file. Will fix. Sorry about that.
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:40: <img src="skin/img/navbar-logo.png" srcset="skin/img/navbar-logo.svg 2x"> On 2017/10/23 14:43:26, martin wrote: > On 2017/10/23 13:27:35, juliandoucette wrote: > > On 2017/10/23 13:17:42, Thomas Greiner wrote: > > > What's the reason for including both PNG as well as SVG files for each icon? > > > > He copied and pasted the new http://abp.org header. It's unclear to me how > much we want > > to deviate from the http://abp.org implementation. That's indeed a tricky question how much it makes sense to deviate from it. Given that those are two different environments with different requirements I'd say we shouldn't try to force it at this point - especially not in a rushed situation like this one - but rather introduce inconsistencies where necessary and document them somewhere so that we can work on solutions later on that work equally well for both UI and Websites. > As far as I know SVGs are served on high-dpi displays, hence the reason for > including them. As @juliandoucette pointed out I simply copy / pasted his > implementation of the header. I'm just wondering why we're not using those SVGs for low-dpi screens as well. Presumably, this may be due to lacking support in older browsers which we don't need to support in the extension. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:41: <span>Adblock <strong>Plus</strong></span> On 2017/10/23 14:43:26, martin wrote: > On 2017/10/23 13:27:34, juliandoucette wrote: > > On 2017/10/23 13:17:42, Thomas Greiner wrote: > > > Please make sure that any text we show in the UI is translatable. That also > > > includes alt-attributes unless they're only there for decoration. > > > > Is it necessary to make text that we do not translate translatable in ABPUI? > (We > > don't translate "Adblock Plus" AFAIK.) > > I intentionally didn't i18n this because it's more or less a brand name / logo. > Not sure whether we should be translating it. You're right. We're not translating "Adblock Plus" in the options page either so feel free to ignore this part of my comment. Note that we usually do include it in our translation files though. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:71: <a href="https://adblockplus.org/ar/firefox" hreflang="ar"> On 2017/10/23 14:43:25, martin wrote: > On 2017/10/23 12:01:55, Thomas Greiner wrote: > > On 2017/10/21 16:18:42, juliandoucette wrote: > > > These hrefs are not correct. We want to translate this page, not goto > > > http://adblockplus.org. The last first run page didn't have a language > > selector. > > > Perhaps @greiner can explain how this page is / can be translated? > > > > IIRC I brought this up in a UI meeting that it doesn't make sense to have the > > language selector in the first-run page. > > > > > Also I'd like to point out that we don't include any links in the extension > > directly but instead use documentation links (see spec) to ensure that links > > don't break. > > What I know for sure is that the spec I'm currently working off of, does indeed > include a language selector. If we decide to not include it in this iteration we > should probably ping Jeen so that she can update the spec? I'm not sure as to > what the proper procedure should be. Yep, filing a spec issue should be sufficient. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... firstRun.html:199: <a href="" class="store-button applestore-button"> On 2017/10/23 14:43:26, martin wrote: > On 2017/10/23 13:17:42, Thomas Greiner wrote: > > Don't we have the links for those buttons yet? > > We probably do, however they are not included in the spec. Will try and clarify > that with Jeen. Great, thanks. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode49 firstRun.js:49: function toggleClass(element, className) On 2017/10/23 13:29:58, juliandoucette wrote: > On 2017/10/23 13:27:35, juliandoucette wrote: > > On 2017/10/23 13:17:43, Thomas Greiner wrote: > > > We've already been using `Element.classList` in other pages so please use it > > > instead of introducing those utility functions for handling CSS classes. > > > > He copied this from http://abp.org main.js. It's unclear to me whether we > should > > re-write this for abpui or just separate it. What do you think? From what I see, if we remove the language selection code, there'd only be one instance left where we actually use it. So I'd say there's little value in keeping it just for the sake of consistency with Websites. > (On that note, we could polyfill classList and refactor main.js in abp.org.) True, good idea.
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode49 firstRun.js:49: function toggleClass(element, className) On 2017/10/23 16:45:49, Thomas Greiner wrote: > > (On that note, we could polyfill classList and refactor main.js in abp.org.) > > True, good idea. See https://codereview.adblockplus.org/29587659
I just submitted a new patch-set for review. https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.js#newcode49 firstRun.js:49: function toggleClass(element, className) On 2017/10/24 14:22:18, juliandoucette wrote: > On 2017/10/23 16:45:49, Thomas Greiner wrote: > > > (On that note, we could polyfill classList and refactor main.js in abp.org.) > > > > True, good idea. > > See https://codereview.adblockplus.org/29587659 Done. https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... File locale/en_US/firstRun.json (right): https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... locale/en_US/firstRun.json:14: "firstRun_title": { On 2017/10/23 13:17:43, Thomas Greiner wrote: > Coding style: "No whitespace at the end of a line." Done. https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... locale/en_US/firstRun.json:15: "message": "INSTALLATION SUCCESSFUL!" On 2017/10/23 13:17:43, Thomas Greiner wrote: > Detail: The capitalization of this text (and others in this file) is styling > information which may change. Therefore we tend not to include the text as if it > weren't in all caps (e.g. "Installation Successful") and apply the style > information separately via CSS (i.e. `text-transform: uppercase`). Completely agree. Will fix. https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... locale/en_US/firstRun.json:24: "message": "By default, you may see some nonintrisive ads that adhere to strict criteria. We identify these ads as Acceptable Ads. Prefer to block all ads? <a>Turn off Acceptable Ads</a> feature in seconds." On 2017/10/23 13:17:43, Thomas Greiner wrote: > Typo: Replace "nonintrinsive" with "nonintrusive". Done. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2017/10/21 16:18:46, juliandoucette wrote: > Please keep the spaces before "*" in these comments At first I wasn't sure what you meant exactly, but now I think you want the spaces in order to make comments collapsable? I'll add spaces so that collapsing would be possible. I actually copied the compiled "css" from website-defaults. The indentation must have been screwed up during the scss->css process. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:18: @font-face On 2017/10/21 16:18:43, juliandoucette wrote: > This doesn't seem to be used anywhere? Done. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:41: font-style: normal; On 2017/10/21 16:18:43, juliandoucette wrote: > NIT: You could name this "bold" Done. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:62: width: 80%; On 2017/10/21 16:18:43, juliandoucette wrote: > new http://abp.org uses website-defaults .container width. Why are you setting a custom > width for this page? Honestly I chose to override the default for the sole purpose of the buttons in the first and second columns to horizontally align on wider displays. Due to the variable height of the <p> the buttons misalign by default. Alternatively we can give the columns a fixed min-height and pin the buttons to the bottom, however that approach felt a bit off. I dropped the width rule. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:64: margin: 0 auto 8em; On 2017/10/21 16:18:44, juliandoucette wrote: > NIT: `.container` is a very general class. It's meant to centre the content > within, and control the width and padding of, horizontal sections of the page. > And it's currently being used in the main content *and* the footer. I'm guessing > that you are trying to add 8em of space between the main content and the footer. > If I am guessing correctly then I would suggest using a more specific selector > for this purpose e.g. Adding an ID to the main content and applying an 8em > margin-bottom to it. Done. https://codereview.adblockplus.org/29565721/diff/29584656/skin/firstRun.css#n... skin/firstRun.css:75: #content-columns-container > .one-third On 2017/10/21 16:18:44, juliandoucette wrote: > NIT: `#content-columns-container` is quite long. We currently use `#content` for > this purpose on http://adblockplus.org. I suggest we use `#content` for the sake of > consistency. Done.
LGTM + NITs I suggest addressing these NITs and re-submitting if it doesn't block anything. I gave the LGTM because I think this is past the point of "Good enough, we can/will address anything else that we find separately." e.g. the navbar contents will change as soon as we have translations. the sidebar/navbar menu may or may not land before this goes into a release. Please wait for saroyanm or greiner's go-ahead before pushing. https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:37: <nav id="navbar"> NIT: I've made several small updates to the navbar, footer, and default.css that will at least: - add large-desktop-width - deprecate .navbar-container - fix the collapsed space under the open navbar menu on mobile - slightly reduce the margin-bottom of the footer Please update these snippets. (https://bitbucket.org/adblockplus/adblockplus.org) https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:73: <div id="main-content-container" class="container content"> NIT/Suggest: Replace the id "main-content-container" with "content" and remove the ">" from the corresponding #content styles. https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:75: <div id="content" class="row"> NIT: I don't think that centred text looks good at desktop width and font-size. I suspect that it will look better at large-desktop-width. But I suggest align-left and/or dropping the font-size at desktop width. https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:77: <section class="column one-third ta-centre"> NIT: If you add ta-center to the parent, all children will inherit it https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css#n... skin/firstRun.css:28: @font-face NIT: new adblockplus.org is currently using semi-bold 600 instead of bold 700 (This may not be documented in the spec/styleguide yet - sorry) https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css#n... skin/firstRun.css:49: .container NIT: Both of these styles are covered by default.css. See https://codereview.adblockplus.org/29565721/diff/29589564/skin/defaults.css#n... https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css#n... skin/firstRun.css:55: #main-content-container { NIT: .container already applies margin 0 auto. It would be clearer/more specific to apply margin-bottom: 8em.
What is the status of this review? AFAIK this page hasn't made it into adblockplusui yet?
On 2017/12/01 16:31:31, juliandoucette wrote: > What is the status of this review? AFAIK this page hasn't made it into > adblockplusui yet? Sorry for not keeping you posted on that. For the last couple of weeks we've been focusing our efforts on the Updates page and are therefore not going to be able to ship the first-run page redesign this year anymore. Unlike what we initially hoped for, quite a few additional changes were necessary to properly integrate such pages into the extension. Note that many of the changes we had to make in the Updates page review also apply to this one. So there's still a bit of work ahead of us here.
Moving myself to CC.
Hey all, After a long long time, I addressed Julian's last round of comments and NITs. I also updated the store button images to be the same as the ones used in the updates page. Please note that I have limited availability to work on this, as I'm spread throughout a number of different projects so, if we want to land this soon, it's probably a good idea for someone else to take over. Cheers! https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:37: <nav id="navbar"> On 2017/10/30 15:02:27, juliandoucette wrote: > NIT: I've made several small updates to the navbar, footer, and default.css that > will at least: > > - add large-desktop-width > - deprecate .navbar-container > - fix the collapsed space under the open navbar menu on mobile > - slightly reduce the margin-bottom of the footer > > Please update these snippets. > > (https://bitbucket.org/adblockplus/adblockplus.org) Done. https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:73: <div id="main-content-container" class="container content"> On 2017/10/30 15:02:27, juliandoucette wrote: > NIT/Suggest: Replace the id "main-content-container" with "content" and remove > the ">" from the corresponding #content styles. Done. https://codereview.adblockplus.org/29565721/diff/29589564/firstRun.html#newco... firstRun.html:77: <section class="column one-third ta-centre"> On 2017/10/30 15:02:27, juliandoucette wrote: > NIT: If you add ta-center to the parent, all children will inherit it Done. https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css#n... skin/firstRun.css:49: .container On 2017/10/30 15:02:28, juliandoucette wrote: > NIT: Both of these styles are covered by default.css. See > https://codereview.adblockplus.org/29565721/diff/29589564/skin/defaults.css#n... Done. https://codereview.adblockplus.org/29565721/diff/29589564/skin/firstRun.css#n... skin/firstRun.css:55: #main-content-container { On 2017/10/30 15:02:28, juliandoucette wrote: > NIT: .container already applies margin 0 auto. It would be clearer/more specific > to apply margin-bottom: 8em. Done.
Sorry for taking so long. The content of firstRun.css is a bit confusing so it'd be great if we could split it up as suggested in one of my comments. That way we won't need to adapt any of the Websites styles because not all of them are relevant for our purposes. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:27: <script type="text/javascript" src="polyfill.js"></script> Detail: The "type" property is optional in HTML5 so, unlike Websites, we can drop them for UI pages. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:39: <a href="https://adblockplus.org/en/" hreflang="en" id="navbar-logo"> As done with the Updates page, don't hardcode any links but instead use documentation links. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:39: <a href="https://adblockplus.org/en/" hreflang="en" id="navbar-logo"> Unlike on the website itself, we don't know whether the web page we're linking to will be fully translated. Therefore let's remove the "hreflang" property from all links. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:39: <a href="https://adblockplus.org/en/" hreflang="en" id="navbar-logo"> All external links should be opened in a new tab. On the website that means that links such as this one should open in the same tab but since we're not on adblockplus.org but in the browser's UI, those links are also considered as external. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:40: <img src="skin/img/navbar-logo.svg"> I noticed that the PNG files are still included in this review so please remove them since we're not using them anyway. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:68: <img src="skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark icon"> Detail: As mentioned in the Updates page review, we don't need to annotate purely decorative images such as this one but also others on this page. Instead let's use `alt=""` so that screen readers can ignore them. See also https://www.w3.org/WAI/tutorials/images/decorative/ https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:73: <div id="content" class="container content"> There are now two elements on this page with the ID "content" so please avoid doing that or otherwise we'll run into issues due to it leading to undefined behavior. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:82: <p id="first-column-description" class="i18n_firstRun_controlDescription column-description"></p> Detail: There are still element IDs which mention the order of the column (i.e. "first-column-description" and "third-column-description"). Furthermore, there are still message IDs which mention the order of the column (i.e. "firstRun_columnOneTitle", "firstRun_columnTwoTitle" and "firstRun_columnThreeTitle"). See also https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newco... https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:93: <a href="" class="store-button applestore-button"> Please add the missing links to the mobile stores. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.html#newco... firstRun.html:94: <img src="skin/icons/first-run/appstore-bg.svg" alt="apple store button"> Let's reuse the app store badge SVGs we added for the Updates page to avoid unnecessary duplication. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js File firstRun.js (right): https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js#newcode18 firstRun.js:18: /* globals checkShareResource, getDocLink, openSharePopup, setLinks, E */ Detail: We no longer use `checkShareResource()` and `openSharePopup()` so no need to list them here. We can even remove those two functions from commons.js now since this was the only page that was using them. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js#newcode31 firstRun.js:31: document.getElementById("navbar-menu").classList.toggle("visible"); Detail: I don't mind using either `E()` or `document.getElementById()` but at least we should be consistent by using either the one or the other but not both. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js#newcode36 firstRun.js:36: document.getElementById("navbar-menu-toggle").addEventListener("click", navigationClick, false); Coding style: "Line length: 80 characters or less" Also applies to line 50. Note that you can now automatically detect and fix coding style issues by running `npm run lint` (see https://github.com/adblockplus/adblockplusui#linting as well as https://github.com/adblockplus/adblockplusui#installing-dependencies for more info on that). https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js#newcode36 firstRun.js:36: document.getElementById("navbar-menu-toggle").addEventListener("click", navigationClick, false); Detail: The third function argument parameter is redundant. We used to pass it in the past which is why it's still used for "DOMContentLoaded" but we have since stopped doing that. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js#newcode41 firstRun.js:41: const optionsTrigger = E("options-trigger"); Coding style: "Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.)." In this case the value of `optionsTrigger` is not a constant expression but a function call. https://codereview.adblockplus.org/29565721/diff/29684663/firstRun.js#newcode49 firstRun.js:49: setLinks("first-column-description", " https://adblockplus.org/terms"); As done with the Updates page, don't hardcode any links but instead use documentation links. https://codereview.adblockplus.org/29565721/diff/29684663/locale/en_US/firstR... File locale/en_US/firstRun.json (right): https://codereview.adblockplus.org/29565721/diff/29684663/locale/en_US/firstR... locale/en_US/firstRun.json:21: "message": "YOU'RE IN CONTROL" Some strings in this file are still in all-caps. See also https://codereview.adblockplus.org/29565721/diff/29584656/locale/en_US/firstR... https://codereview.adblockplus.org/29565721/diff/29684663/locale/en_US/firstR... locale/en_US/firstRun.json:87: "message": "Copyright © 2017 All rights reserved. Adblock Plus® is a registered trademark of <a>eyeo GmbH.</a>" Detail: Let's not hardcode the year because that means that we'd have to update this page once per year. Instead, what we do in our license headers is write it as "Copyright (C) 2006-present eyeo GmbH" so we could rewrite this here as "Copyright © 2006-present All rights reserved. Adblock Plus® is a registe red trademark of <a>eyeo GmbH.</a>" if we think it's necessary to even mention the year. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:1: /* It is difficult to distinguish styles that are coming from websites-defaults from styles you introduced for the purpose of the first-run page. However, both are mixed together in "firstRun.css", implying that all of those were made specifically for the first-run page. Therefore, I strongly suggest to split those styles up and only include styles specifically created for the first-run page in here. Thereby you can keep any non-relevant styles (e.g. ones targeting IE9) but in a separate file so that we know that we've simply imported them from Websites and don't need to change them. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:16: */ Detail: What happened to the whitespaces here? https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:72: *****************************************************************************/ Detail: Missing whitespace at beginning of line causing the line to be offset a bit. Also applies to other occurrences in this file. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:80: font-weight: bold; We consistently use number values for the font weight across all our files to avoid confusion so please use `700` here. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:81: margin: 32px 0px; Coding style: "Omit unit specification after “0” values, unless required." Also applies to other occurrences in this file. See https://google.github.io/styleguide/htmlcssguide.html#0_and_Units https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:244: vertical-align: .255em; Coding style: "Don't omit the optional leading 0 for decimal numbers." Also applies to other occurrences in this file. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:312: /* #navbar #navbar-menu #navbar-locale-menu You don't include this section of the navbar so let's not include its styles either. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:348: float: left; I don't see a rule for reversing this for right-to-left locales. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:379: background: #f9f9f9; Detail: Do you actually want to override other background-* properties or do you merely want to change the color? Because for the latter, using "background-color" would make this more maintainable. Also applies to other occurrences in this file. https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:398: color: #fff !important; What's the reason for using `!important` here? https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:450: .applestore-button Coding style: "Separate rules by new lines." See https://google.github.io/styleguide/htmlcssguide.html#Rule_Separation https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:486: #footer h5:after Detail: The standard way to write this is `#footer h5::after`. Unlike Websites, we don't need to support older browsers who don't understand the standard syntax yet. See also https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements https://codereview.adblockplus.org/29565721/diff/29684663/skin/firstRun.css#n... skin/firstRun.css:582: /* IE9+ only */ This is not relevant for us since we don't support IE9 so let's remove it. 
|
