Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

No 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+1329 lines, -604 lines) Patch
M firstRun.html View 1 2 3 4 1 chunk +154 lines, -72 lines 10 comments Download
M firstRun.js View 1 2 3 1 chunk +27 lines, -91 lines 6 comments Download
M locale/en_US/firstRun.json View 1 2 3 1 chunk +90 lines, -30 lines 2 comments Download
A skin/defaults.css View 1 2 3 4 1 chunk +446 lines, -0 lines 0 comments Download
M skin/firstRun.css View 1 2 3 4 1 chunk +522 lines, -411 lines 13 comments Download
A skin/icons/first-run/appstore-bg.svg View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A skin/icons/first-run/googleplay-bg.svg View 1 2 3 4 1 chunk +41 lines, -0 lines 0 comments Download
A skin/icons/first-run/icon-checkmark.png View Binary file 0 comments Download
A skin/icons/first-run/icon-checkmark.svg View 1 1 chunk +1 line, -0 lines 0 comments Download
A skin/icons/first-run/icon-checkmark-header.svg View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A skin/icons/first-run/icon-lock.png View Binary file 0 comments Download
A skin/icons/first-run/icon-lock.svg View 1 1 chunk +1 line, -0 lines 0 comments Download
A skin/icons/first-run/icon-rocket.png View Binary file 0 comments Download
A skin/icons/first-run/icon-rocket.svg View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A skin/img/footer-facebook-glyphicon.png View 1 Binary file 0 comments Download
A skin/img/footer-instagram-glyphicon.png View 1 Binary file 0 comments Download
A skin/img/footer-twitter-glyphicon.png View 1 Binary file 0 comments Download
A skin/img/footer-youtube-glyphicon.png View 1 Binary file 0 comments Download
A skin/img/menu-toggle.png View 1 Binary file 0 comments Download
A skin/img/menu-toggle.svg View 1 1 chunk +43 lines, -0 lines 0 comments Download
A skin/img/navbar-logo.png View 1 Binary file 0 comments Download
A skin/img/navbar-logo.svg View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 30
martin
Oct. 5, 2017, 1:47 p.m. (2017-10-05 13:47:36 UTC) #1
juliandoucette
Here are my first impressions. Also: - Please updating coding-style according to https://adblockplus.org/coding-style (you may ...
Oct. 8, 2017, 1:18 p.m. (2017-10-08 13:18:12 UTC) #2
martin
Here's a couple of your comments addressed. I think I largely have the HTML where ...
Oct. 16, 2017, 9:09 a.m. (2017-10-16 09:09:10 UTC) #3
juliandoucette
https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newcode57 firstRun.html:57: <a href="" class="store-button applestore-button"></a> On 2017/10/16 09:09:09, martin wrote: ...
Oct. 16, 2017, 10 a.m. (2017-10-16 10:00:38 UTC) #4
saroyanm
I reviewed only comment Julian asked me, but in general I think Module owner or ...
Oct. 16, 2017, 10:22 a.m. (2017-10-16 10:22:59 UTC) #5
saroyanm
Please also include an issue, or at least relevant links to the review.
Oct. 16, 2017, 10:36 a.m. (2017-10-16 10:36:20 UTC) #6
juliandoucette
Added Thomas as reviewer.
Oct. 16, 2017, 11:52 a.m. (2017-10-16 11:52:34 UTC) #7
martin
https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29565722/firstRun.html#newcode36 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: ...
Oct. 16, 2017, 2:36 p.m. (2017-10-16 14:36:00 UTC) #8
juliandoucette
The latest PatchSet does not apply. Can you please updated and rebase?
Oct. 17, 2017, 11:19 a.m. (2017-10-17 11:19:13 UTC) #9
juliandoucette
https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29581817/firstRun.html#newcode49 firstRun.html:49: <a href="/en/about" hreflang="en">About</a> Missing translations (the same applies elsewhere) ...
Oct. 18, 2017, 10:42 a.m. (2017-10-18 10:42:38 UTC) #10
martin
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#newcode77 skin/firstRun.css:77: .one-third On 2017/10/18 10:42:37, juliandoucette wrote: > NIT: This ...
Oct. 20, 2017, 10:03 a.m. (2017-10-20 10:03:29 UTC) #11
juliandoucette
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#newcode77 skin/firstRun.css:77: .one-third On 2017/10/20 10:03:28, martin wrote: > On ...
Oct. 20, 2017, 1:50 p.m. (2017-10-20 13:50:22 UTC) #12
juliandoucette
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#newcode99 skin/firstRun.css:99: .content h1 { font-size: 2.4em; ...
Oct. 20, 2017, 2:13 p.m. (2017-10-20 14:13:52 UTC) #13
martin
Hey all! I addressed most of the last round's feedback. I (hopefully) managed to submit ...
Oct. 20, 2017, 2:46 p.m. (2017-10-20 14:46:36 UTC) #14
juliandoucette
On 2017/10/20 14:46:36, martin wrote: > Hey all! > > I addressed most of the ...
Oct. 21, 2017, 3:37 p.m. (2017-10-21 15:37:07 UTC) #15
juliandoucette
@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#newcode71 firstRun.html:71: <a href="https://adblockplus.org/ar/firefox" hreflang="ar"> ...
Oct. 21, 2017, 4:18 p.m. (2017-10-21 16:18:46 UTC) #16
Thomas Greiner
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newcode71 firstRun.html:71: <a href="https://adblockplus.org/ar/firefox" hreflang="ar"> On 2017/10/21 16:18:42, juliandoucette wrote: > ...
Oct. 23, 2017, 12:01 p.m. (2017-10-23 12:01:56 UTC) #17
Thomas Greiner
I quickly looked through the code and noticed a couple of things that caught my ...
Oct. 23, 2017, 1:17 p.m. (2017-10-23 13:17:45 UTC) #18
juliandoucette
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#newcode40 firstRun.html:40: <img src="skin/img/navbar-logo.png" ...
Oct. 23, 2017, 1:27 p.m. (2017-10-23 13:27:36 UTC) #19
juliandoucette
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, ...
Oct. 23, 2017, 1:29 p.m. (2017-10-23 13:29:59 UTC) #20
martin
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newcode40 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: ...
Oct. 23, 2017, 2:43 p.m. (2017-10-23 14:43:28 UTC) #21
Thomas Greiner
https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29565721/diff/29584656/firstRun.html#newcode40 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: ...
Oct. 23, 2017, 4:45 p.m. (2017-10-23 16:45:49 UTC) #22
juliandoucette
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: ...
Oct. 24, 2017, 2:22 p.m. (2017-10-24 14:22:18 UTC) #23
martin
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 ...
Oct. 26, 2017, 10:33 a.m. (2017-10-26 10:33:20 UTC) #24
juliandoucette
LGTM + NITs I suggest addressing these NITs and re-submitting if it doesn't block anything. ...
Oct. 30, 2017, 3:02 p.m. (2017-10-30 15:02:29 UTC) #25
juliandoucette
What is the status of this review? AFAIK this page hasn't made it into adblockplusui ...
Dec. 1, 2017, 4:31 p.m. (2017-12-01 16:31:31 UTC) #26
Thomas Greiner
On 2017/12/01 16:31:31, juliandoucette wrote: > What is the status of this review? AFAIK this ...
Dec. 1, 2017, 5:22 p.m. (2017-12-01 17:22:29 UTC) #27
juliandoucette
Moving myself to CC.
Jan. 4, 2018, 3:27 p.m. (2018-01-04 15:27:23 UTC) #28
martin
Hey all, After a long long time, I addressed Julian's last round of comments and ...
Jan. 30, 2018, 2:10 p.m. (2018-01-30 14:10:05 UTC) #29
Thomas Greiner
Feb. 16, 2018, 4:19 p.m. (2018-02-16 16:19:21 UTC) #30
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.

Powered by Google App Engine
This is Rietveld