updates.html - Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension

Delta Between Two Patch Sets: updates.html

Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension (Closed) Base URL: https://hg.adblockplus.org/adblockplusui/

Left Patch Set: Addressed third round of feedback Created Nov. 20, 2017, 11:44 a.m.

Right Patch Set: Addressed seventh round of feedback Created Jan. 12, 2018, 11:35 a.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_updates_status"></title>	21 <title class="i18n_updates_status"></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 href="skin/updates.css" rel="stylesheet"/>	24 <link href="skin/updates.css" rel="stylesheet"/>

25 <script src="polyfill.js"></script>	25 <script src="polyfill.js"></script>

26 <script src="ext/common.js"></script>	26 <script src="ext/common.js"></script>

27 <script src="ext/content.js"></script>	27 <script src="ext/content.js"></script>

28 <script src="common.js"></script>	28 <script src="common.js"></script>

29 <script src="i18n.js"></script>	29 <script src="i18n.js"></script>

30 <script src="updates.js"></script>	30 <script src="updates.js"></script>
Thomas Greiner 2017/11/20 17:06:07 This file is not included in the review so please This file is not included in the review so please make sure to add it. martin 2017/11/26 17:08:40 Done. Show quoted text On 2017/11/20 17:06:07, Thomas Greiner wrote: > This file is not included in the review so please make sure to add it. Done.
31 </head>	31 </head>

32	32

33 <body>	33 <body>

34	34

35 <main>	35 <main>

36 <div id="container">	36 <div id="container">

37	37

38 <header class="column graphic-column">	38 <header class="column graphic-column">

39 <div class="update-graphic-container">	39 <div class="update-graphic-container">

40 <div class="update-graphic-content">	40 <div class="update-graphic-content">

41 <img src="/skin/abp-logo.svg"/>	41 <img src="/skin/abp-logo.svg"/>

42 <div class="version-details">	42 <div class="version-details">

43 <h2 class="i18n_updates_status"></h2>	43 <h2 class="i18n_updates_status"></h2>

44 </div>	44 </div>

45	45

46 </div>	46 </div>

47 </header>	47 </header>

48	48

49 <section class="column content-column">	49 <section class="column content-column">

50	50

51 <div id="content">	51 <div id="content">

52	52

53 <header>	53 <header>

54 <h1 class="i18n_updates_heading"></h1>	54 <h1 class="i18n_updates_heading"></h1>

55 <p class="i18n_updates_subheading"></p>	55 <p class="i18n_updates_subheading"></p>

56 </header>	56 </header>

57	57

58 <article class="feature-entry">	58 <article class="feature-entry">

59 <img class="feature-icon" src="/skin/updates/icon-rocket.svg" al t="rocket-icon"/>	59 <img class="feature-icon" src="/skin/updates/icon-rocket.svg"/>

60 <p id="optionsPageNews" class="i18n_updates_optionsPageNews"></p >	60 <p id="options-page-news" class="i18n_updates_optionsPageNews">< /p>
Thomas Greiner 2017/11/20 17:06:07 Coding style: "Separate words in ID and class name Coding style: "Separate words in ID and class names by a hyphen." See https://adblockplus.org/en/coding-style#html-css and https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... martin 2017/11/26 17:08:40 Done. Show quoted text On 2017/11/20 17:06:07, Thomas Greiner wrote: > Coding style: "Separate words in ID and class names by a hyphen." > > See https://adblockplus.org/en/coding-style#html-css and > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... Done.
61 </article>	61 </article>

62	62

63 <article class="feature-entry">	63 <article class="feature-entry">

64 <img class="feature-icon" src="/skin/updates/icon-thumbs-up.svg" alt="thumbs-up-icon"/>	64 <img class="feature-icon" src="/skin/updates/icon-thumbs-up.svg" />

65 <p id="block-ads-text" class="i18n_updates_blockAds"></p>	65 <p id="block-ads-text" class="i18n_updates_blockAds"></p>

66 </article>	66 </article>

67	67

68 <article class="custom-feature-entry">	68 <article class="custom-feature-entry">

69 <h2 class="i18n_updates_phone"></h2>	69 <h2 class="i18n_updates_phone"></h2>

	70

70 <div class="feature-entry">	71 <div class="feature-entry">

71 <img class="feature-icon" src="/skin/updates/icon-mobile.svg" alt="thumbs-up-icon"/>	72 <img class="feature-icon" src="/skin/updates/icon-mobile.svg"/ >

72 <p class="i18n_updates_adblockBrowser"></p>	73 <p id="adblock-browser-text" class="i18n_updates_adblockBrowse r"></p>

73 </div>	74 </div>

	75

74 <div class="store-buttons">	76 <div class="store-buttons">

75 <a href="https://eyeo.to/adblockbrowser/ios/update-page" class ="store-button applestore-button">	77 <a id="adblock-browser-ios-store" class="store-button applesto re-button" target="_blank">
Thomas Greiner 2017/11/20 17:06:07 We use documentation links (see https://bitbucket. We use documentation links (see https://bitbucket.org/adblockplus/spec/src/3a5d8435c857976db8e4b58d18148504f4...) so we set any links using JavaScript. Looks like those links haven't been added to the spec yet though so we need to make sure to add them before we roll out the update. Example: https://hg.adblockplus.org/adblockplusui/file/tip/mobile-options.js#l435 martin 2017/11/26 17:08:39 I'm sort of confused as to how to use this. The `g Show quoted text On 2017/11/20 17:06:07, Thomas Greiner wrote: > We use documentation links (see > https://bitbucket.org/adblockplus/spec/src/3a5d8435c857976db8e4b58d18148504f4...) > so we set any links using JavaScript. > > Looks like those links haven't been added to the spec yet though so we need to > make sure to add them before we roll out the update. > > Example: > https://hg.adblockplus.org/adblockplusui/file/tip/mobile-options.js#l435 I'm sort of confused as to how to use this. The `getDocLink` basically selects the actual <a> DOM element. But then we set the `href` attribute to the `link` argument. Here's where I'm lost. Where is this "link" arg defined? Can't I just select the element and set its href to "http://something.something"? Out of curiosity - why are these utility functions necessary? Thomas Greiner 2017/11/27 16:15:47 No, the `getDocLink()` function gives you the URL Show quoted text On 2017/11/26 17:08:39, martin wrote: > I'm sort of confused as to how to use this. The `getDocLink` basically selects > the actual <a> DOM element. But then we set the `href` attribute to the `link` > argument. Here's where I'm lost. Where is this "link" arg defined? No, the `getDocLink()` function gives you the URL for a given ID, not the `<a>` element. Example: getDocLink("id", (url) => { element.href = url; }); So for the ID "releases", `"id"` would be `"releases"`, `url` would be `https://adblockplus.org/redirect?link=releases&lang=en-US` and `element` would be the `<a>` element. Show quoted text > Can't I just > select the element and set its href to "http://something.something"? Out of > curiosity - why are these utility functions necessary? It's possible to do it like this but there are two benefits to this approach: 1) By using adblockplus.org/redirect we can ensure that we don't link to web pages that no longer exist. Unlike with websites, links that we include in the extension are there forever so linking to our own website allows us to dynamically change the link destination without having to release an extension update. 2) Centralizing the creation of this link using `getDocLink()` allows us to avoid code duplication. martin 2017/12/05 14:37:29 Aaaaah! I get it now. Thanks for the explanation. Show quoted text On 2017/11/27 16:15:47, Thomas Greiner wrote: > On 2017/11/26 17:08:39, martin wrote: > > I'm sort of confused as to how to use this. The `getDocLink` basically selects > > the actual <a> DOM element. But then we set the `href` attribute to the `link` > > argument. Here's where I'm lost. Where is this "link" arg defined? > > No, the `getDocLink()` function gives you the URL for a given ID, not the `<a>` > element. > > Example: > getDocLink("id", (url) => > { > element.href = url; > }); > > So for the ID "releases", `"id"` would be `"releases"`, `url` would be > `https://adblockplus.org/redirect?link=releases&lang=en-US` and `element` would > be the `<a>` element. > > > Can't I just > > select the element and set its href to "http://something.something"? Out of > > curiosity - why are these utility functions necessary? > > It's possible to do it like this but there are two benefits to this approach: > > 1) By using adblockplus.org/redirect we can ensure that we don't link to web > pages that no longer exist. Unlike with websites, links that we include in the > extension are there forever so linking to our own website allows us to > dynamically change the link destination without having to release an extension > update. > > 2) Centralizing the creation of this link using `getDocLink()` allows us to > avoid code duplication. Aaaaah! I get it now. Thanks for the explanation. It makes sense now.
76 <img src="skin/updates/appstore-bg.svg" alt="apple store but ton">	78 <img src="skin/updates/appstore-bg.svg" alt="apple store but ton">

77 </a>	79 </a>

78 <a href="https://eyeo.to/adblockbrowser/android/update-page" c lass="store-button googleplay-button">	80 <a id="adblock-browser-android-store" class="store-button goog leplay-button" target="_blank">

79 <img src="skin/updates/googleplay-bg.svg" alt="google play s tore button">	81 <img src="skin/updates/googleplay-bg.svg" alt="google play s tore button">

80 </a>	82 </a>

81 </div>	83 </div>

	84

82 </article>	85 </article>

83	86

84 </div>	87 </div>

85	88

86 </section>	89 </section>

87	90

88 </div>	91 </div>

89	92

90 </main>	93 </main>

91	94

92 </body>	95 </body>

93	96

94 </html>	97 </html>

LEFT	RIGHT