firstRun.html - Issue 29565721: No Issue - Implemented first run page

Delta Between Two Patch Sets: firstRun.html

Issue 29565721: No Issue - Implemented first run page Base URL: https://hg.adblockplus.org/adblockplusui/

Left Patch Set: Addressed newest round of feedback (2) Created Oct. 26, 2017, 10:29 a.m.

Right Patch Set: Addressed latest round of feedback Created Jan. 30, 2018, 2:06 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <!--	2 <!--

3 - This file is part of Adblock Plus <https://adblockplus.org/>,	3 - This file is part of Adblock Plus <https://adblockplus.org/>,

4 - Copyright (C) 2006-present eyeo GmbH	4 - Copyright (C) 2006-present eyeo GmbH

5 -	5 -

6 - Adblock Plus is free software: you can redistribute it and/or modify	6 - Adblock Plus is free software: you can redistribute it and/or modify

7 - it under the terms of the GNU General Public License version 3 as	7 - it under the terms of the GNU General Public License version 3 as

8 - published by the Free Software Foundation.	8 - published by the Free Software Foundation.

9 -	9 -

10 - Adblock Plus is distributed in the hope that it will be useful,	10 - Adblock Plus is distributed in the hope that it will be useful,

11 - but WITHOUT ANY WARRANTY; without even the implied warranty of	11 - but WITHOUT ANY WARRANTY; without even the implied warranty of

12 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	12 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

13 - GNU General Public License for more details.	13 - GNU General Public License for more details.

14 -	14 -

15 - You should have received a copy of the GNU General Public License	15 - You should have received a copy of the GNU General Public License

16 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	16 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

17 -->	17 -->

18	18

19 <html>	19 <html>

20 <head>	20 <head>

21 <title class="i18n_firstRun_title"></title>	21 <title class="i18n_firstRun_title"></title>

22 <meta charset="utf-8" />	22 <meta charset="utf-8" />

23 <meta name="viewport" content="width=device-width, initial-scale=1" />	23 <meta name="viewport" content="width=device-width, initial-scale=1" />

24 <link type="text/css" href="skin/common.css" rel="stylesheet" />	24 <link type="text/css" href="skin/common.css" rel="stylesheet" />

25 <link type="text/css" href="skin/defaults.css" rel="stylesheet" />	25 <link type="text/css" href="skin/defaults.css" rel="stylesheet" />

26 <link type="text/css" href="skin/firstRun.css" rel="stylesheet"/>	26 <link type="text/css" href="skin/firstRun.css" rel="stylesheet"/>

27 <script type="text/javascript" src="polyfill.js"></script>	27 <script type="text/javascript" src="polyfill.js"></script>
	Thomas Greiner 2018/02/16 16:19:10 Detail: The "type" property is optional in HTML5 s Detail: The "type" property is optional in HTML5 so, unlike Websites, we can drop them for UI pages.
28 <script type="text/javascript" src="ext/common.js"></script>	28 <script type="text/javascript" src="ext/common.js"></script>

29 <script type="text/javascript" src="ext/content.js"></script>	29 <script type="text/javascript" src="ext/content.js"></script>

30 <script type="text/javascript" src="common.js"></script>	30 <script type="text/javascript" src="common.js"></script>

31 <script type="text/javascript" src="i18n.js"></script>	31 <script type="text/javascript" src="i18n.js"></script>

32 <script type="text/javascript" src="firstRun.js"></script>	32 <script type="text/javascript" src="firstRun.js"></script>

33 </head>	33 </head>

34	34

35 <body>	35 <body>

36	36

37 <nav id="navbar">	37 <nav id="navbar">
juliandoucette 2017/10/30 15:02:27 NIT: I've made several small updates to the 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) martin 2018/01/30 14:10:02 Done. Show quoted text 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.
38 <div class="navbar-container">	38 <div class="navbar-container">

39 <a href="https://adblockplus.org/en/" hreflang="en" id="navbar-logo">	39 <a href="https://adblockplus.org/en/" hreflang="en" id="navbar-logo">
	Thomas Greiner 2018/02/16 16:19:08 As done with the Updates page, don't hardcode any As done with the Updates page, don't hardcode any links but instead use documentation links. Thomas Greiner 2018/02/16 16:19:09 Unlike on the website itself, we don't know whethe 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. Thomas Greiner 2018/02/16 16:19:10 All external links should be opened in a new tab. 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.
40 <img src="skin/img/navbar-logo.svg">	40 <img src="skin/img/navbar-logo.svg">
	Thomas Greiner 2018/02/16 16:19:08 I noticed that the PNG files are still included in I noticed that the PNG files are still included in this review so please remove them since we're not using them anyway.
41 <span>Adblock <strong>Plus</strong></span>	41 <span>Adblock <strong>Plus</strong></span>

42 </a>	42 </a>

43 <a href="#" id="navbar-menu-toggle">	43 <a href="#" id="navbar-menu-toggle">

44 <img src="skin/img/menu-toggle.svg">	44 <img src="skin/img/menu-toggle.svg">

45 </a>	45 </a>

46 <ul id="navbar-menu">	46 <ul id="navbar-menu">

47 <li>	47 <li>

48 <a href="https://adblockplus.org/en/about" hreflang="en" class="i18n_f irstRun_aboutLink"></a>	48 <a href="https://adblockplus.org/en/about" hreflang="en" class="i18n_f irstRun_aboutLink"></a>

49 </li>	49 </li>

50	50

51 <li>	51 <li>

52 <a href="https://adblockplus.org/en/features" hreflang="en" class="i18 n_firstRun_featuresLink"></a>	52 <a href="https://adblockplus.org/en/features" hreflang="en" class="i18 n_firstRun_featuresLink"></a>

53 </li>	53 </li>

54	54

55 <li>	55 <li>

56 <a href="https://adblockplus.org/en/bugs" hreflang="en" class="i18n_fi rstRun_reportAnIssueLink"></a>	56 <a href="https://adblockplus.org/en/bugs" hreflang="en" class="i18n_fi rstRun_reportAnIssueLink"></a>

57 </li>	57 </li>

58	58

59 <li>	59 <li>

60 <a href="https://adblockplus.org/en/contribute" hreflang="en" class="i 18n_firstRun_contributeLink"></a>	60 <a href="https://adblockplus.org/en/contribute" hreflang="en" class="i 18n_firstRun_contributeLink"></a>

61 </li>	61 </li>

62 </ul>	62 </ul>

63 </div>	63 </div>

64 </nav>	64 </nav>

65	65

66 <main>	66 <main>

67 <header id="installation-status-header">	67 <header id="installation-status-header">

68 <img src="skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark i con">	68 <img src="skin/icons/first-run/icon-checkmark-header.svg" alt="checkmark i con">
	Thomas Greiner 2018/02/16 16:19:09 Detail: As mentioned in the Updates page review, w 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/
69 <h1 class="i18n_firstRun_title"></h1>	69 <h1 class="i18n_firstRun_title"></h1>

70 <p class="i18n_firstRun_subtitle"></p>	70 <p class="i18n_firstRun_subtitle"></p>

71 </header>	71 </header>

72	72

73 <div id="main-content-container" class="container content">	73 <div id="content" class="container content">
juliandoucette 2017/10/30 15:02:27 NIT/Suggest: Replace the id "main-content-containe NIT/Suggest: Replace the id "main-content-container" with "content" and remove the ">" from the corresponding #content styles. martin 2018/01/30 14:10:02 Done. Show quoted text 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.	Thomas Greiner 2018/02/16 16:19:09 There are now two elements on this page with the I 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.
74	74

75 <div id="content" class="row">	75 <div id="content" class="row ta-centre">
juliandoucette 2017/10/30 15:02:27 NIT: I don't think that centred text looks good at 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.
76	76

77 <section class="column one-third ta-centre">	77 <section class="column one-third">
juliandoucette 2017/10/30 15:02:27 NIT: If you add ta-center to the parent, all child NIT: If you add ta-center to the parent, all children will inherit it martin 2018/01/30 14:10:03 Done. Show quoted text On 2017/10/30 15:02:27, juliandoucette wrote: > NIT: If you add ta-center to the parent, all children will inherit it Done.
78 <header class="section-header">	78 <header class="section-header">

79 <img src="skin/icons/first-run/icon-checkmark.svg" alt="checkmark ic on">	79 <img src="skin/icons/first-run/icon-checkmark.svg" alt="checkmark ic on">

80 <h2 class="i18n_firstRun_columnOneTitle"></h2>	80 <h2 class="i18n_firstRun_columnOneTitle"></h2>

81 </header>	81 </header>

82 <p id="first-column-description" class="i18n_firstRun_controlDescripti on column-description"></p>	82 <p id="first-column-description" class="i18n_firstRun_controlDescripti on column-description"></p>
	Thomas Greiner 2018/02/16 16:19:08 Detail: There are still element IDs which mention 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...
83 <a id="options-trigger" class="i18n_firstRun_configure-button button-p rimary" href="#"></a>	83 <a id="options-trigger" class="i18n_firstRun_configure-button button-p rimary" href="#"></a>

84 </section>	84 </section>

85	85

86 <section class="column one-third ta-centre">	86 <section class="column one-third">

87 <header class="section-header">	87 <header class="section-header">

88 <img src="skin/icons/first-run/icon-rocket.svg" alt="rocket icon">	88 <img src="skin/icons/first-run/icon-rocket.svg" alt="rocket icon">

89 <h2 class="i18n_firstRun_columnTwoTitle"></h2>	89 <h2 class="i18n_firstRun_columnTwoTitle"></h2>

90 </header>	90 </header>

91 <p class="i18n_firstRun_browserAppDescription column-description"></p>	91 <p class="i18n_firstRun_browserAppDescription column-description"></p>

92 <div class="store-buttons">	92 <div class="store-buttons">

93 <a href="" class="store-button applestore-button">	93 <a href="" class="store-button applestore-button">
	Thomas Greiner 2018/02/16 16:19:11 Please add the missing links to the mobile stores. Please add the missing links to the mobile stores.
94 <img src="skin/icons/first-run/appstore-bg.svg" alt="apple store b utton">	94 <img src="skin/icons/first-run/appstore-bg.svg" alt="apple store b utton">
	Thomas Greiner 2018/02/16 16:19:10 Let's reuse the app store badge SVGs we added for Let's reuse the app store badge SVGs we added for the Updates page to avoid unnecessary duplication.
95 </a>	95 </a>

96 <a href="" class="store-button googleplay-button">	96 <a href="" class="store-button googleplay-button">

97 <img src="skin/icons/first-run/googleplay-bg.svg" alt="google play store button">	97 <img src="skin/icons/first-run/googleplay-bg.svg" alt="google play store button">

98 </a>	98 </a>

99 </div>	99 </div>

100 </section>	100 </section>

101	101

102 <section class="column one-third ta-centre">	102 <section class="column one-third">

103 <header class="section-header">	103 <header class="section-header">

104 <img src="skin/icons/first-run/icon-lock.svg" alt="padlock icon">	104 <img src="skin/icons/first-run/icon-lock.svg" alt="padlock icon">

105 <h2 class="i18n_firstRun_columnThreeTitle"></h2>	105 <h2 class="i18n_firstRun_columnThreeTitle"></h2>

106 </header>	106 </header>

107 <p id="third-column-description" class="i18n_firstRun_fairUseDescripti on column-description"></p>	107 <p id="third-column-description" class="i18n_firstRun_fairUseDescripti on column-description"></p>

108 </section>	108 </section>

109 </div>	109 </div>

110 </div>	110 </div>

111 </main>	111 </main>

112	112

(...skipping 59 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
172 <li><a href="https://adblockplus.org/en/privacy" class="i18n_firstRun_ footerPrivacyPolicyLink" hreflang="en"></a></li>	172 <li><a href="https://adblockplus.org/en/privacy" class="i18n_firstRun_ footerPrivacyPolicyLink" hreflang="en"></a></li>

173 <li><a href="https://adblockplus.org/en/impressum" class="i18n_firstRu n_footerLegalNoticeLink" hreflang="en"></a></li>	173 <li><a href="https://adblockplus.org/en/impressum" class="i18n_firstRu n_footerLegalNoticeLink" hreflang="en"></a></li>

174 </ul>	174 </ul>

175 </div>	175 </div>

176 </div>	176 </div>

177 </footer>	177 </footer>

178	178

179 </body>	179 </body>

180	180

181 </html>	181 </html>

LEFT	RIGHT