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: Created Oct. 5, 2017, 1:47 p.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/firstRun.css" rel="stylesheet"/>	25 <link type="text/css" href="skin/defaults.css" rel="stylesheet" />

26 <script type="text/javascript" src="ext/common.js"></script>	26 <link type="text/css" href="skin/firstRun.css" rel="stylesheet"/>

27 <script type="text/javascript" src="ext/content.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="common.js"></script>	28 <script type="text/javascript" src="ext/common.js"></script>

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

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

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

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

	33 </head>

32	34

33 <body>	35 <body>
juliandoucette 2017/10/08 13:18:10 NIT: You should wrap the content currently on this 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) martin 2017/10/16 09:09:08 Done. Show quoted text 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.
	36

	37 <nav id="navbar">

	38 <div class="navbar-container">

	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">
	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>

	42 </a>

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

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

	45 </a>

	46 <ul id="navbar-menu">

	47 <li>

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

	49 </li>

34	50

35 <header>	51 <li>

36 <img src="./skin/icons/first-run/icon-checkmark-header.svg" alt="checkma rk icon">	52 <a href="https://adblockplus.org/en/features" hreflang="en" class="i18 n_firstRun_featuresLink"></a>
juliandoucette 2017/10/08 13:18:10 NIT: Alt text should be translated (The same appl NIT: Alt text should be translated (The same applies elsewhere) juliandoucette 2017/10/08 13:18:10 NIT: "./" is unnecessary here (The same applies e NIT: "./" is unnecessary here (The same applies elsewhere) martin 2017/10/16 09:09:08 I honestly don't know how to do that with the curr Show quoted text 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 :/ saroyanm 2017/10/16 10:22:59 text inside of attributes can't be easily translat Show quoted text 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. martin 2017/10/16 14:36:00 Done. Show quoted text 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.
37 <h1 class="i18n_firstRun_title"></h1>	53 </li>

38 <p class="i18n_firstRun_subtitle"></p>	54

	55 <li>

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

	57 </li>

	58

	59 <li>

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

	61 </li>

	62 </ul>

	63 </div>

	64 </nav>

	65

	66 <main>

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

	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>

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

39 </header>	71 </header>

40	72

41 <div class="container content">	73 <div id="content" class="container content">
	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.
42	74

	75 <div id="content" class="row ta-centre">

	76

	77 <section class="column one-third">

	78 <header class="section-header">

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

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

	81 </header>

	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>

	84 </section>

	85

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

	87 <header class="section-header">

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

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

	90 </header>

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

	92 <div class="store-buttons">

	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">
	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>

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

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

	98 </a>

	99 </div>

	100 </section>

	101

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

	103 <header class="section-header">

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

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

	106 </header>

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

	108 </section>

	109 </div>

	110 </div>

	111 </main>

	112

	113 <footer id="footer" class="content">

	114 <div class="container">

43 <div class="row">	115 <div class="row">

44	116 <nav class="column one-fourth">

45 <div class="column one-third">	117 <h5 class="i18n_firstRun_footerResourcesHeader"></h5>
juliandoucette 2017/10/08 13:18:10 NIT: These (columns) should be <article>s (See ht 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).) martin 2017/10/16 09:09:08 Done. Show quoted text 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.
46 <img src="./skin/icons/first-run/icon-checkmark.svg" alt="checkmark ic on">	118 <ul>

47 <h2 class="i18n_firstRun_columnOneTitle"></h2>	119 <li><a href="https://adblockplus.org/en/acceptable-ads" class="i18n_ firstRun_footerAcceptableAdsLink" hreflang="en"></a></li>
juliandoucette 2017/10/08 13:18:10 NIT: I think that you should wrap the <img> and th 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. martin 2017/10/16 09:09:09 Done. Show quoted text 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.
48 <p id="first-column-description" class="i18n_firstRun_columnOneDescrip tion"></p>	120 <li><a href="https://adblockplus.org/en/documentation" class="i18n_f irstRun_footerDocumentationLink" hreflang="en"></a></li>

49 <a id="options-trigger" class="i18n_firstRun_configure-button button-p rimary" href="#"></a>	121 <li><a href="https://adblockplus.org/en/deployments" class="i18n_fir stRun_footerForAdminsLink" hreflang="en"></a></li>

50 </div>	122 </ul>

51	123 </nav>

52 <div class="column one-third">	124 <nav class="column one-fourth">

53 <img src="./skin/icons/first-run/icon-rocket.svg" alt="rocket icon">	125 <h5 class="i18n_firstRun_footerCommunityHeader"></h5>

54 <h2 class="i18n_firstRun_columnTwoTitle"></h2>	126 <ul>

55 <p class="i18n_firstRun_columnTwoDescription"></p>	127 <li><a href="https://adblockplus.org/releases/" class="i18n_firstRun _footerAnnouncementsLink" hreflang="en"></a></li>

56 <div class="store-buttons">	128 <li><a href="https://adblockplus.org/blog/" class="i18n_firstRun_foo terBlogLink" hreflang="en"></a></li>

57 <a href="" class="store-button applestore-button"></a>	129 <li><a href="https://adblockplus.org/forum/" class="i18n_firstRun_fo oterForumLink" hreflang="en"></a></li>
juliandoucette 2017/10/08 13:18:10 NIT: There is no text in this button You could us 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.) martin 2017/10/16 09:09:09 Soo... I think what you mean is that instead of Show quoted text 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? juliandoucette 2017/10/16 10:00:38 ~Yes. I suggested the following solutions: 1. Pl Show quoted text 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.) martin 2017/10/16 14:36:00 Done. I went for option 2. Show quoted text 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.
58 <a href="" class="store-button googleplay-button"></a>	130 <li><a href="https://adblockplus.org/en/development-builds" class="i 18n_firstRun_footerDevelopmentBuildsLink" hreflang="en"></a></li>

59 </div>	131 </ul>

60 </div>	132 </nav>

61	133 <nav class="column one-fourth">

62 <div class="column one-third">	134 <h5 class="i18n_firstRun_footerDevelopmentHeader"></h5>

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

64 <h2 class="i18n_firstRun_columnThreeTitle"></h2>	136 <li><a href="https://adblockplus.org/en/source" class="i18n_firstRun _footerSourceCodeLink" hreflang="en"></a></li>

65 <p id="third-column-description" class="i18n_firstRun_columnThreeDes cription"></p>	137 <li><a href="https://issues.adblockplus.org/report/13" class="i18n_f irstRun_footerRoadmapLink" hreflang="en"></a></li>

66 </div>	138 <li><a href="https://adblockplus.org/en/tools" class="i18n_firstRun_ footerToolsLink" hreflang="en"></a></li>

67	139 <li><a href="https://eyeo.com/jobs/" class="i18n_firstRun_footerJobs Link" hreflang="en"></a></li>

68 </div>	140 </ul>

69	141 </nav>

70 </div>	142 <nav id="social-list" class="column one-fourth">

71	143 <h5 class="i18n_firstRun_footerFollowUsHeader"></h5>

72 </body>	144 <ul>

	145 <li>

	146 <a href="https://twitter.com/AdblockPlus" target="_blank">

	147 <img src="skin/img/footer-twitter-glyphicon.png" alt="Twitter">

	148 </a>

	149 </li>

	150 <li>

	151 <a href="https://www.youtube.com/user/AdblockPlusOfficial" target= "_blank">

	152 <img src="skin/img/footer-youtube-glyphicon.png">

	153 </a>

	154 </li>

	155 <li>

	156 <a href="https://www.facebook.com/adblockplus" target="_blank">

	157 <img src="skin/img/footer-facebook-glyphicon.png">

	158 </a>

	159 </li>

	160 <li>

	161 <a href="https://www.instagram.com/adblockplus/" target="_blank">

	162 <img src="skin/img/footer-instagram-glyphicon.png">

	163 </a>

	164 </li>

	165 </ul>

	166 </nav>

	167 </div>

	168 <div id="footer-legal">

	169 <p id="copyright-notice" class="i18n_firstRun_footerCopyright"></p>

	170 <ul id="legal-list">

	171 <li><a href="https://adblockplus.org/en/terms" class="i18n_firstRun_fo oterTermsOfUseLink" 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>

	174 </ul>

	175 </div>

	176 </div>

	177 </footer>

	178

	179 </body>

73	180

74 </html>	181 </html>

LEFT	RIGHT