Issue 29722640: Fixes #22 - Add download page (Closed)
|
Created:
March 14, 2018, 1:31 p.m. by ire Modified:
April 3, 2018, 3:41 p.m. Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
DescriptionFixes #22 - Add download page
Issue - https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/22
Branch - index_page
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressed comments #3 #
Total comments: 21
Patch Set 3 : Resize abb logo, use linkify function #Patch Set 4 : Rebase #
Total comments: 15
Patch Set 5 : Addressed comments #17 #
MessagesTotal messages: 21
Hey Julian! I'm waiting for the features section review to be done to update that, but here is my first stab at it. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:2: description=Get Adblock Plus for free on Firefox, Chrome, Opera, Safari, Android and iOS The title & description aren't in the spec yet. See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/22#note_63233215 https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:65: <div class="row item-group"> Until we push your features review, I'm using the item-group layout here https://codereview.adblockplus.org/29722640/diff/29722641/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29722640/diff/29722641/static/css/main.css... static/css/main.css:805: /* Horizontal List Taken from help.eyeo.com. I'm thinking we should add this to website-defaults?
https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:2: description=Get Adblock Plus for free on Firefox, Chrome, Opera, Safari, Android and iOS On 2018/03/14 13:34:37, ire wrote: > The title & description aren't in the spec yet. > > See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/22#note_63233215 Acknowledged. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:19: "description": "By clicking any of the links below, you agree to our <a href=\"terms\">Terms of Use</a>", Missing period. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:58: alt="White down arrow"> Missing translation. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:59: <h1>{{ "Get Adblock Plus products on these devices" | translate("download-heading") }}</h1> Missing string context (heading). https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:60: <p>{{ "Supported platforms" | translate("download-subheading") }}</p> Missing string context (subheading). https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:64: <div class="container text-center content"> Suggest: section class? https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:65: <div class="row item-group"> On 2018/03/14 13:34:37, ire wrote: > Until we push your features review, I'm using the item-group layout here Acknowledged. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:73: alt="{{ device.icon_alt }}"> Missing translation. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:74: <h2 class="item-heading">{{ device.title }}</h2> Missing translation. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:76: <p>{{ device.description | safe }}</p> Missing translation. I didn't know "| safe" was a thing... Cool. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:77: <ul class="horizontal-list"> Detail: We may consider optimizing these links using a schema e.g. Product (Can be done separately.) https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:79: <li> See https://gitlab.com/eyeo/specs/spec/merge_requests/135#note_63279602 https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:80: <a href="{{ platform.url }}">{{ platform.name }}</a>{{ ", " if loop.index != loop.length }} Missing translation. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:80: <a href="{{ platform.url }}">{{ platform.name }}</a>{{ ", " if loop.index != loop.length }} Detail: We may consider optimizing these link labels and/or titles for search engines e.g. "Adblock Plus for Samsung browser" is better than "Samsung browser". (Can be done separately.) https://codereview.adblockplus.org/29722640/diff/29722641/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29722640/diff/29722641/static/css/main.css... static/css/main.css:805: /* Horizontal List On 2018/03/14 13:34:37, ire wrote: > Taken from http://help.eyeo.com. I'm thinking we should add this to website-defaults? Agreed.
New patch set https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:19: "description": "By clicking any of the links below, you agree to our <a href=\"terms\">Terms of Use</a>", On 2018/03/14 17:54:53, juliandoucette wrote: > Missing period. Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:58: alt="White down arrow"> On 2018/03/14 17:54:52, juliandoucette wrote: > Missing translation. Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:59: <h1>{{ "Get Adblock Plus products on these devices" | translate("download-heading") }}</h1> On 2018/03/14 17:54:52, juliandoucette wrote: > Missing string context (heading). Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:60: <p>{{ "Supported platforms" | translate("download-subheading") }}</p> On 2018/03/14 17:54:52, juliandoucette wrote: > Missing string context (subheading). Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:64: <div class="container text-center content"> On 2018/03/14 17:54:52, juliandoucette wrote: > Suggest: section class? That would create a different sectioning context. Also, there would be no heading for this section. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:73: alt="{{ device.icon_alt }}"> On 2018/03/14 17:54:53, juliandoucette wrote: > Missing translation. Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:74: <h2 class="item-heading">{{ device.title }}</h2> On 2018/03/14 17:54:52, juliandoucette wrote: > Missing translation. Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:76: <p>{{ device.description | safe }}</p> On 2018/03/14 17:54:52, juliandoucette wrote: > Missing translation. Done. > I didn't know "| safe" was a thing... Cool. Yeah! https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:79: <li> On 2018/03/14 17:54:53, juliandoucette wrote: > See https://gitlab.com/eyeo/specs/spec/merge_requests/135#note_63279602 Done. https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl... pages/download.tmpl:80: <a href="{{ platform.url }}">{{ platform.name }}</a>{{ ", " if loop.index != loop.length }} On 2018/03/14 17:54:52, juliandoucette wrote: > Missing translation. Done. https://codereview.adblockplus.org/29722640/diff/29722641/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29722640/diff/29722641/static/css/main.css... static/css/main.css:805: /* Horizontal List On 2018/03/14 17:54:53, juliandoucette wrote: > On 2018/03/14 13:34:37, ire wrote: > > Taken from http://help.eyeo.com. I'm thinking we should add this to > website-defaults? > > Agreed. https://issues.adblockplus.org/ticket/6484
@Manvel since Julian is away could you please have a look at this for me? Notes: - Make sure you're on the index_page branch - The specifications for this change are here (https://gitlab.com/eyeo/specs/spec/blob/8cebc0dfa2df0b9e22a806d092f2ec9d824be...) - If there are any discrepancies with the spec, you may want to check the discussion here (https://gitlab.com/eyeo/specs/spec/merge_requests/135)
On 2018/03/16 09:12:24, ire wrote: > @Manvel since Julian is away could you please have a look at this for me? > > Notes: > - Make sure you're on the index_page branch I think I'm, but I'm getting rejection when applying the patch. > - The specifications for this change are here > (https://gitlab.com/eyeo/specs/spec/blob/8cebc0dfa2df0b9e22a806d092f2ec9d824be...) > - If there are any discrepancies with the spec, you may want to check the > discussion here (https://gitlab.com/eyeo/specs/spec/merge_requests/135) Noted, thanks.
On 2018/03/16 15:48:34, saroyanm wrote: > On 2018/03/16 09:12:24, ire wrote: > > @Manvel since Julian is away could you please have a look at this for me? > > > > Notes: > > - Make sure you're on the index_page branch > I think I'm, but I'm getting rejection when applying the patch. Nevermind, a user error, I had another patch applied.
Thanks Ire, first round is ready. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:1: title=Download Adblock Plus I assume having 2 steps installation from the homepage in comparison to previous 1 click installation from the homepage is temporary, something we plan to change ? Also in some cases it's 3 step installation. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:20: "description": "By clicking any of the links below, you agree to our <a href=\"terms\">Terms of Use</a>.", Detail: We used to use a special function when we were creating a relative links called "linkify". This function suppose to generate link and set hreflang attribute as well. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, Is that intentional that we are not using inline installation -> https://developer.chrome.com/webstore/inline_installation ? Make sense to check for other extensions as well. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:27: { "name": "Opera", "url": "https://addons.opera.com/extensions/details/opera-adblock/?display=en-US" }, Detail: No need to use language specific version of the page. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:45: "icons": ["abb-icon.png", "abb-icon-2x.png"], The small icon is 500px on 500px, as far as I can see we are using 90x90px on the page, either make sense to use 180px for both size, or adjust accordingly, until we don't show the larger icon on this page. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:49: { "name": "Adblock Browser (Android)", "url": "https://play.google.com/store/apps/details?id=org.adblockplus.browser" }, We can use redirection links instead: https://adblockplus.org/redirect?link=adblock_browser_android_store and https://adblockplus.org/redirect?link=adblock_browser_ios_store Make sense to find out which of this items should also use redirection links, I mean which of the items that are not using inline installation. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:86: <a href="{{ platform.url }}">{{ platform.name | translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index != loop.length }} Detail: There are some Browser names that are localized, for example in Russian it's Яндекс.Браузер https://browser.yandex.ru/, but I'm not sure if we would like to localize our product names, ex.: Adblock Browser and there might be some other product names we would like to avoid mistake translations. Probable make sense to ask Tamara and if so maybe we can make a use of the fix tags as well.
Thanks Manvel! New patch uploaded I've moved some of your comments here https://gitlab.com/eyeo/specs/spec/merge_requests/121#note_64068129 so they can be addressed by Jeen/Tamara. When they get back to us I'll update this review again https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:1: title=Download Adblock Plus On 2018/03/19 19:04:07, saroyanm wrote: > I assume having 2 steps installation from the homepage in comparison to previous > 1 click installation from the homepage is temporary, something we plan to change > ? > > Also in some cases it's 3 step installation. This download page is more of a fallback in case the appropriate browser isn't detected. See https://codereview.adblockplus.org/29727563/ https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:20: "description": "By clicking any of the links below, you agree to our <a href=\"terms\">Terms of Use</a>.", On 2018/03/19 19:04:07, saroyanm wrote: > Detail: We used to use a special function when we were creating a relative links > called "linkify". This function suppose to generate link and set hreflang > attribute as well. Good suggestion! Done. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/19 19:04:07, saroyanm wrote: > Is that intentional that we are not using inline installation -> > https://developer.chrome.com/webstore/inline_installation ? > > Make sense to check for other extensions as well. I will bring this up with Jeen https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:27: { "name": "Opera", "url": "https://addons.opera.com/extensions/details/opera-adblock/?display=en-US" }, On 2018/03/19 19:04:07, saroyanm wrote: > Detail: No need to use language specific version of the page. Done. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:45: "icons": ["abb-icon.png", "abb-icon-2x.png"], On 2018/03/19 19:04:07, saroyanm wrote: > The small icon is 500px on 500px, as far as I can see we are using 90x90px on > the page, either make sense to use 180px for both size, or adjust accordingly, > until we don't show the larger icon on this page. You're right. i just copied the images from the spec but I should have resized them. Done. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:49: { "name": "Adblock Browser (Android)", "url": "https://play.google.com/store/apps/details?id=org.adblockplus.browser" }, On 2018/03/19 19:04:07, saroyanm wrote: > We can use redirection links instead: > https://adblockplus.org/redirect?link=adblock_browser_android_store > and > https://adblockplus.org/redirect?link=adblock_browser_ios_store > > Make sense to find out which of this items should also use redirection links, I > mean which of the items that are not using inline installation. Will bring this up with Jeen https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:86: <a href="{{ platform.url }}">{{ platform.name | translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index != loop.length }} On 2018/03/19 19:04:06, saroyanm wrote: > Detail: There are some Browser names that are localized, for example in Russian > it's Яндекс.Браузер https://browser.yandex.ru/, but I'm not sure if we would > like to localize our product names, ex.: Adblock Browser and there might be some > other product names we would like to avoid mistake translations. > > Probable make sense to ask Tamara and if so maybe we can make a use of the fix > tags as well. I will bring this up with Tamara
On 2018/03/20 07:25:27, ire wrote: > I've moved some of your comments here > https://gitlab.com/eyeo/specs/spec/merge_requests/121#note_64068129 so they can > be addressed by Jeen/Tamara. When they get back to us I'll update this review > again I commented on the wrong issue :/ Here's the link to the correct comment - https://gitlab.com/eyeo/specs/spec/merge_requests/135#note_64068407
@Manvel responses from Jeen and Tamara : https://gitlab.com/eyeo/specs/spec/merge_requests/135 https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/20 07:25:25, ire wrote: > On 2018/03/19 19:04:07, saroyanm wrote: > > Is that intentional that we are not using inline installation -> > > https://developer.chrome.com/webstore/inline_installation ? > > > > Make sense to check for other extensions as well. > > I will bring this up with Jeen > From Jeen's response it seems like the links here are the correct ones. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:86: <a href="{{ platform.url }}">{{ platform.name | translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index != loop.length }} On 2018/03/20 07:25:25, ire wrote: > On 2018/03/19 19:04:06, saroyanm wrote: > > Detail: There are some Browser names that are localized, for example in > Russian > > it's Яндекс.Браузер https://browser.yandex.ru/, but I'm not sure if we would > > like to localize our product names, ex.: Adblock Browser and there might be > some > > other product names we would like to avoid mistake translations. > > > > Probable make sense to ask Tamara and if so maybe we can make a use of the fix > > tags as well. > > I will bring this up with Tamara According to Tamara we should not be using the fix tags here.
Sorry for late reply, was busy with the ABPUI release process. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:1: title=Download Adblock Plus On 2018/03/20 07:25:26, ire wrote: > On 2018/03/19 19:04:07, saroyanm wrote: > > I assume having 2 steps installation from the homepage in comparison to > previous > > 1 click installation from the homepage is temporary, something we plan to > change > > ? > > > > Also in some cases it's 3 step installation. > > This download page is more of a fallback in case the appropriate browser isn't > detected. > > See https://codereview.adblockplus.org/29727563/ Noted https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/20 14:40:58, ire wrote: > On 2018/03/20 07:25:25, ire wrote: > > On 2018/03/19 19:04:07, saroyanm wrote: > > > Is that intentional that we are not using inline installation -> > > > https://developer.chrome.com/webstore/inline_installation ? > > > > > > Make sense to check for other extensions as well. > > > > I will bring this up with Jeen > > > > From Jeen's response it seems like the links here are the correct ones. Yes, but in order the inline installation for the desktop product to work we should include <link href="https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" rel="chrome-webstore-item"> right ? Which seems like is missing here. For other webstores that should be different ex. for Firefox, but make sense to be sure that the installation is 1 step, until this was intentional (according to Jeen's comment this wasn't ?) https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:86: <a href="{{ platform.url }}">{{ platform.name | translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index != loop.length }} On 2018/03/20 14:40:58, ire wrote: > On 2018/03/20 07:25:25, ire wrote: > > On 2018/03/19 19:04:06, saroyanm wrote: > > > Detail: There are some Browser names that are localized, for example in > > Russian > > > it's Яндекс.Браузер https://browser.yandex.ru/, but I'm not sure if we would > > > like to localize our product names, ex.: Adblock Browser and there might be > > some > > > other product names we would like to avoid mistake translations. > > > > > > Probable make sense to ask Tamara and if so maybe we can make a use of the > fix > > > tags as well. > > > > I will bring this up with Tamara > > According to Tamara we should not be using the fix tags here. Acknowledged. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:51: { "name": "Adblock Browser (Android)", "url": "https://play.google.com/store/apps/details?id=org.adblockplus.browser" }, I noticed that we still not using redirection links, so I also replied in the gitlab discussion.
Sorry for late reply, was busy with the ABPUI release process.
https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/23 18:33:38, saroyanm wrote: > On 2018/03/20 14:40:58, ire wrote: > > On 2018/03/20 07:25:25, ire wrote: > > > On 2018/03/19 19:04:07, saroyanm wrote: > > > > Is that intentional that we are not using inline installation -> > > > > https://developer.chrome.com/webstore/inline_installation ? > > > > > > > > Make sense to check for other extensions as well. > > > > > > I will bring this up with Jeen > > > > > > > From Jeen's response it seems like the links here are the correct ones. > > Yes, but in order the inline installation for the desktop product to work we > should include <link > href="https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" > rel="chrome-webstore-item"> right ? Which seems like is missing here. > > For other webstores that should be different ex. for Firefox, but make sense to > be sure that the installation is 1 step, until this was intentional (according > to Jeen's comment this wasn't ?) Okay I think I was mixing up two different things here, sorry about that. I don't think we are doing any browser detection on the download page, since it's main function is as a fallback for when we don't detect the browser from the homepage. Because of that, I don't think the inline installers are being used here, we are just linking to the various web stores. On the homepage, where we are doing the browser detection, is where I think the one-click installers are being used. I will double-check this with Jeen though, as I don't think her response was clear. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:51: { "name": "Adblock Browser (Android)", "url": "https://play.google.com/store/apps/details?id=org.adblockplus.browser" }, On 2018/03/23 18:33:39, saroyanm wrote: > I noticed that we still not using redirection links, so I also replied in the > gitlab discussion. Ack. Will wait for Jeen's response.
https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/26 07:29:07, ire wrote: > On 2018/03/23 18:33:38, saroyanm wrote: > > On 2018/03/20 14:40:58, ire wrote: > > > On 2018/03/20 07:25:25, ire wrote: > > > > On 2018/03/19 19:04:07, saroyanm wrote: > > > > > Is that intentional that we are not using inline installation -> > > > > > https://developer.chrome.com/webstore/inline_installation ? > > > > > > > > > > Make sense to check for other extensions as well. > > > > > > > > I will bring this up with Jeen > > > > > > > > > > From Jeen's response it seems like the links here are the correct ones. > > > > Yes, but in order the inline installation for the desktop product to work we > > should include <link > > > href="https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" > > rel="chrome-webstore-item"> right ? Which seems like is missing here. > > > > For other webstores that should be different ex. for Firefox, but make sense > to > > be sure that the installation is 1 step, until this was intentional (according > > to Jeen's comment this wasn't ?) > > Okay I think I was mixing up two different things here, sorry about that. > > I don't think we are doing any browser detection on the download page, since > it's main function is as a fallback for when we don't detect the browser from > the homepage. Because of that, I don't think the inline installers are being > used here, we are just linking to the various web stores. On the homepage, where > we are doing the browser detection, is where I think the one-click installers > are being used. > > I will double-check this with Jeen though, as I don't think her response was > clear. So Jeen confirmed that we are not using inline installations on the download page, just the homepage. See https://gitlab.com/eyeo/specs/spec/merge_requests/135#note_65041707
On 2018/03/23 18:33:42, saroyanm wrote: > Sorry for late reply, was busy with the ABPUI release process. No problem :)
LGTM + NITs Don't forget to optimize images if they aren't already. https://codereview.adblockplus.org/29722640/diff/29729581/includes/features.html File includes/features.html (right): https://codereview.adblockplus.org/29722640/diff/29729581/includes/features.h... includes/features.html:1: <div class="item-group container content"> uh...? https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:22: "description": "By clicking any of the links below, you agree to our " + "terms" | linkify + "Terms of Use</a>.", NIT/Suggest: <small> (semantic, not size.) > The HTML <small> element makes the text font size one size smaller (for example, from large to medium, or from small to x-small) down to the browser's minimum font size. In HTML5, this element is repurposed to represent side-comments and small print, including copyright and legal text, independent of its styled presentation. > > -- mdn https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:72: <div class="column one-third"> NIT/Suggest: <article> > The HTML <article> element represents a self-contained composition in a document, page, application, or site, which is intended to be independently distributable or reusable (e.g., in syndication). Examples include: a forum post, a magazine or newspaper article, or a blog entry. > > -- mdn https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:77: srcset="/img/{{ device.icons[1] }} 2x" NIT: You could use a dict and place device.icons.1x and 2x to be more descriptive. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:78: alt="{{ device.icon_alt | translate(device.id + "-icon-alt", "Alt text") }}"> NIT: I usually imagine "id"s to be for machines and "name"s to be for humans. And it seems like you're referring to a human kind of "id" (a "name"). I think this is a PHP convention. You may consider adopting it if you like the idea. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:79: <h2>{{ device.title | translate(device.id + "-title", "Heading") }}</h2> NIT: It's unnecessary to capitalize string contexts. (The same applies elsewhere.) https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:88: <a href="{{ platform.url }}">{{ platform.name | translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index != loop.length }} Cool :+1:
RE: "uh...?" - I'm not sure if you meant to include the item-group change in here. If you did, perhaps you could push it as a separate commit?
On 2018/04/03 13:27:10, juliandoucette wrote: > RE: "uh...?" - I'm not sure if you meant to include the item-group change in > here. If you did, perhaps you could push it as a separate commit? I did mean to include this change here, but I will push it separately.
On 2018/04/03 15:03:41, ire wrote: > On 2018/04/03 13:27:10, juliandoucette wrote: > > RE: "uh...?" - I'm not sure if you meant to include the item-group change in > > here. If you did, perhaps you could push it as a separate commit? > > I did mean to include this change here, but I will push it separately. Done https://hg.adblockplus.org/web.adblockplus.org/rev/a207ad736fa6
Thanks Julian! I've addressed your comments and will push https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:22: "description": "By clicking any of the links below, you agree to our " + "terms" | linkify + "Terms of Use</a>.", On 2018/04/03 13:24:51, juliandoucette wrote: > NIT/Suggest: <small> (semantic, not size.) > > > The HTML <small> element makes the text font size one size smaller (for > example, from large to medium, or from small to x-small) down to the browser's > minimum font size. In HTML5, this element is repurposed to represent > side-comments and small print, including copyright and legal text, independent > of its styled presentation. > > > > -- mdn > Ack. Done. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:72: <div class="column one-third"> On 2018/04/03 13:24:51, juliandoucette wrote: > NIT/Suggest: <article> > > > The HTML <article> element represents a self-contained composition in a > document, page, application, or site, which is intended to be independently > distributable or reusable (e.g., in syndication). Examples include: a forum > post, a magazine or newspaper article, or a blog entry. > > > > -- mdn I don't think this fits within the definition of an <article> I don't think it is self-contained. Each "device" seems to be to be part of the rest of the page, under the page title "Get Adblock Plus products on these devices". I don't think each device would be considered to be standalone. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:77: srcset="/img/{{ device.icons[1] }} 2x" On 2018/04/03 13:24:51, juliandoucette wrote: > NIT: You could use a dict and place device.icons.1x and 2x to be more > descriptive. Done. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:78: alt="{{ device.icon_alt | translate(device.id + "-icon-alt", "Alt text") }}"> On 2018/04/03 13:24:51, juliandoucette wrote: > NIT: I usually imagine "id"s to be for machines and "name"s to be for humans. > And it seems like you're referring to a human kind of "id" (a "name"). I think > this is a PHP convention. You may consider adopting it if you like the idea. I look at "id"s simply as "identifiers". I wouldn't consider "abb" for example, to be a name, it's a simplified version of the name that I'm using an an identifier. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:79: <h2>{{ device.title | translate(device.id + "-title", "Heading") }}</h2> On 2018/04/03 13:24:50, juliandoucette wrote: > NIT: It's unnecessary to capitalize string contexts. > > (The same applies elsewhere.) I like it capitalised. It conveys to me that it is more of a sentence/description rather than a single word/identifier. https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... pages/download.tmpl:88: <a href="{{ platform.url }}">{{ platform.name | translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index != loop.length }} On 2018/04/03 13:24:51, juliandoucette wrote: > Cool :+1: Thanks :p 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
