Issue 29727563: Fixes #35 - Progressively enhance install button with appropriate links and text (Closed)
|
Created:
March 19, 2018, 10:25 a.m. by ire Modified:
April 16, 2018, 4:24 p.m. Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
DescriptionFixes #35 - Progressively enhance install button with appropriate links and text
Issue - https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/35
Specification - https://gitlab.com/eyeo/specs/spec/blob/67d3451e3c78148090191ab6df2a3549b0054a4a/spec/adblockplus.org/website.md#hero-unit
Branch: index_page
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 12
Patch Set 3 : Added inline installation #Patch Set 4 : Rebase #
Total comments: 14
Patch Set 5 : Addressed comments #7 #
Total comments: 13
Patch Set 6 : Addressed comments #10 #
Total comments: 11
Patch Set 7 : Addressed comments #12 #
Total comments: 4
Patch Set 8 : Addressed #14 #Patch Set 9 : Change translation strings #
Total comments: 9
Patch Set 10 : Addressed comments #21 #
MessagesTotal messages: 23
@Manvel If you get a chance please take a look at this
First round is ready. https://codereview.adblockplus.org/29727563/diff/29729574/includes/hero-downl... File includes/hero-download.html (right): https://codereview.adblockplus.org/29727563/diff/29729574/includes/hero-downl... includes/hero-download.html:21: <header class="column one-half content show-on-maxthon"> We also have a special disclaimer for the French version of the landing page, I assume we are still planing to include that on the page ? https://codereview.adblockplus.org/29727563/diff/29729574/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29729574/includes/index.tmpl... includes/index.tmpl:18: {{ "Install for <fix>browser</fix>" | translate("install-for-browser", "butt The installation "visited" state of this button is barely visible. https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js#... static/js/index.js:5: var supportedPlatforms = { I wonder what SEO implications this can introduce, as we used to have a separate page for each browser, but now we are introducing 1 page instead? Are we going to generate pages for each platform, or introduce redirections ? Maybe having a separate page for each browser can help us with SEO ? I assume all this was discussed, so would be great to send a link or, maybe we can discuss the implications. https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js#... static/js/index.js:8: installer: "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb", Seems like here we also don't have inline installation, same as here -> https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js#... static/js/index.js:39: installer: "https://eyeo.to/adblockbrowser/ios/abp-website", We might want to use this or similar redirection links in the download page I assume, relevant -> https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... https://codereview.adblockplus.org/29727563/diff/29729574/static/js/vendor/bo... File static/js/vendor/bowser.js (right): https://codereview.adblockplus.org/29727563/diff/29729574/static/js/vendor/bo... static/js/vendor/bowser.js:1: /*! I wonder if it will be fairly simple to keep user agent sniffing the way we used to do in the backend -> https://hg.adblockplus.org/infrastructure/file/tip/modules/web/templates/adbl... Seems like that what bowser does right, just comparing navigator.userAgent ?
New patch set up https://codereview.adblockplus.org/29727563/diff/29729574/includes/hero-downl... File includes/hero-download.html (right): https://codereview.adblockplus.org/29727563/diff/29729574/includes/hero-downl... includes/hero-download.html:21: <header class="column one-half content show-on-maxthon"> On 2018/03/23 19:08:36, saroyanm wrote: > We also have a special disclaimer for the French version of the landing page, I > assume we are still planing to include that on the page ? Yes. We have a separate issue for that (https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/19) https://codereview.adblockplus.org/29727563/diff/29729574/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29729574/includes/index.tmpl... includes/index.tmpl:18: {{ "Install for <fix>browser</fix>" | translate("install-for-browser", "butt On 2018/03/23 19:08:36, saroyanm wrote: > The installation "visited" state of this button is barely visible. This issue was fixed in https://codereview.adblockplus.org/29720653/ and should be merged soon https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js#... static/js/index.js:5: var supportedPlatforms = { On 2018/03/23 19:08:37, saroyanm wrote: > I wonder what SEO implications this can introduce, as we used to have a separate > page for each browser, but now we are introducing 1 page instead? > Are we going to generate pages for each platform, or introduce redirections ? > Maybe having a separate page for each browser can help us with SEO ? > I assume all this was discussed, so would be great to send a link or, maybe we > can discuss the implications. This is a good point. As I wasn't part of the initial conversations around the abp redesign, I'm not aware of if this was discussed. Julian may be able to answer your question or, if not, we can bring this up in the issue. https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js#... static/js/index.js:8: installer: "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb", On 2018/03/23 19:08:37, saroyanm wrote: > Seems like here we also don't have inline installation, same as here -> > https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl... Done. https://codereview.adblockplus.org/29727563/diff/29729574/static/js/index.js#... static/js/index.js:39: installer: "https://eyeo.to/adblockbrowser/ios/abp-website", On 2018/03/23 19:08:37, saroyanm wrote: > We might want to use this or similar redirection links in the download page I > assume, relevant -> > https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl... Ack. https://codereview.adblockplus.org/29727563/diff/29729574/static/js/vendor/bo... File static/js/vendor/bowser.js (right): https://codereview.adblockplus.org/29727563/diff/29729574/static/js/vendor/bo... static/js/vendor/bowser.js:1: /*! On 2018/03/23 19:08:37, saroyanm wrote: > I wonder if it will be fairly simple to keep user agent sniffing the way we used > to do in the backend -> > https://hg.adblockplus.org/infrastructure/file/tip/modules/web/templates/adbl... > Seems like that what bowser does right, just comparing navigator.userAgent ? You're correct that that is what essentially bowser does. I chose to use bowser here and the help center because if I were to write my own browser detection, it would essentially be the same as bowser's. I think they have come up with a good and robust solution, so I don't think we would gain much from starting from scratch ourselves. Granted, there is some detection code in bowser that we don't need (e.g. detection for browsers we don't support), but I don't think this makes a significant difference to us in terms of file size, so I don't really see a downside to using it.
Patch does not apply. Please rebase.
On 2018/04/03 13:41:35, juliandoucette wrote: > Patch does not apply. Please rebase. Done
Thanks Ire! It looks like there is still implementation left to do. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:1: "use strict"; Why is this necessary? https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:6: // Desktop browsers NIT: Please put one blank line above to make this more noticable https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:53: for (var key in supportedPlatforms) if (bowser[key]) detectedPlatform = key; NITs - It took me a minute to figure out what you're doing here... A comment would have helped. - You could break after you find the detected platform Here is a clever alternative way to implement this: var detectedPlatform = Object.keys(supportedPlatforms).find(hasOwnProperty.bind(bowser)); Your implementation may perform better than mine if you break after finding the detectedPlatform. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:65: .replace("browser", supportedPlatforms[detectedPlatform].label); I think the browser name itself is translated in some languages. Please check with C&T if you haven't already. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:65: .replace("browser", supportedPlatforms[detectedPlatform].label); "browser" is not fixed in the include. Therefore, you cannot rely on "browser" existing in the string. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:67: heroDownloadButton.addEventListener("click", function(event) I think chrome.webstore.install works on other chrome based browsers?
On 2018/04/04 10:18:23, juliandoucette wrote: > It looks like there is still implementation left to do. (I was referring to the translation compatibility issues that I pointed out... It may be wise to test this by creating fake translations by manually populating locale files.)
New patch set ready! https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:1: "use strict"; On 2018/04/04 10:18:23, juliandoucette wrote: > Why is this necessary? I originally added it because it was in the main.js file, but it also turns out it's in our style guide to always use strict mode https://adblockplus.org/en/coding-style#javascript (Looks like I'll have to go add this to other JS files as well) https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:6: // Desktop browsers On 2018/04/04 10:18:23, juliandoucette wrote: > NIT: Please put one blank line above to make this more noticable Done. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:53: for (var key in supportedPlatforms) if (bowser[key]) detectedPlatform = key; On 2018/04/04 10:18:23, juliandoucette wrote: > NITs > > - It took me a minute to figure out what you're doing here... A comment would > have helped. > - You could break after you find the detected platform > > Here is a clever alternative way to implement this: > > var detectedPlatform = > Object.keys(supportedPlatforms).find(hasOwnProperty.bind(bowser)); > > Your implementation may perform better than mine if you break after finding the > detectedPlatform. I'll go with your implementation. Done. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:65: .replace("browser", supportedPlatforms[detectedPlatform].label); On 2018/04/04 10:18:23, juliandoucette wrote: > I think the browser name itself is translated in some languages. Please check > with C&T if you haven't already. Done. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:65: .replace("browser", supportedPlatforms[detectedPlatform].label); On 2018/04/04 10:18:23, juliandoucette wrote: > "browser" is not fixed in the include. Therefore, you cannot rely on "browser" > existing in the string. Done. https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:67: heroDownloadButton.addEventListener("click", function(event) On 2018/04/04 10:18:23, juliandoucette wrote: > I think chrome.webstore.install works on other chrome based browsers? You're right. I switch to checking for the "chrome" variable itself https://codereview.adblockplus.org/29727563/diff/29743690/globals/browsers.py File globals/browsers.py (right): https://codereview.adblockplus.org/29727563/diff/29743690/globals/browsers.py... globals/browsers.py:16: browsers = [ TOL: This could be in website-defaults?
Thanks! Almost there! https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:1: "use strict"; On 2018/04/05 14:32:43, ire wrote: > On 2018/04/04 10:18:23, juliandoucette wrote: > > Why is this necessary? > > I originally added it because it was in the main.js file, but it also turns out > it's in our style guide to always use strict mode > https://adblockplus.org/en/coding-style#javascript > > (Looks like I'll have to go add this to other JS files as well) Ack. I don't really think it's necessary. Therefore I'd like to reassess before we add it to other JS files as well. (But we should leave it here because it's in our code style guide.) https://codereview.adblockplus.org/29727563/diff/29743690/globals/browsers.py File globals/browsers.py (right): https://codereview.adblockplus.org/29727563/diff/29743690/globals/browsers.py... globals/browsers.py:16: browsers = [ On 2018/04/05 14:32:44, ire wrote: > TOL: This could be in website-defaults? It could be. Not for adblockplus.org though (until we change how it is deployed). https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:10: label: "Chrome" Are these labels still being used? https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:51: var detectedPlatform = Object.keys(supportedPlatforms).find(hasOwnProperty.bind(bowser)); NIT: Line is too long? https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:57: if (detectedPlatform === "maxthon") return; NIT: Code style says we should use == :( https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:67: if (!chrome) return; It's possible for chrome to exist without webstore (e.g. when running the test server or a modified version of chrome). It's also possible for chrome.webstore.install to fail/error (e.g. because the webstore has invalidated our domain or the user canceled the install). We may decide to handle the success or failure of the install separately (I'm sure Jeen would be interested). I suggest that we check chrome and chrome.webstore before preventing the default behaviour of this event.
Thanks Julian! New patch set https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29741628/static/js/index.js#... static/js/index.js:1: "use strict"; On 2018/04/06 12:35:22, juliandoucette wrote: > On 2018/04/05 14:32:43, ire wrote: > > On 2018/04/04 10:18:23, juliandoucette wrote: > > > Why is this necessary? > > > > I originally added it because it was in the main.js file, but it also turns > out > > it's in our style guide to always use strict mode > > https://adblockplus.org/en/coding-style#javascript > > > > (Looks like I'll have to go add this to other JS files as well) > > Ack. > > I don't really think it's necessary. Therefore I'd like to reassess before we > add it to other JS files as well. > > (But we should leave it here because it's in our code style guide.) Acknowledged. https://codereview.adblockplus.org/29727563/diff/29743690/globals/browsers.py File globals/browsers.py (right): https://codereview.adblockplus.org/29727563/diff/29743690/globals/browsers.py... globals/browsers.py:16: browsers = [ On 2018/04/06 12:35:23, juliandoucette wrote: > On 2018/04/05 14:32:44, ire wrote: > > TOL: This could be in website-defaults? > > It could be. Not for http://adblockplus.org though (until we change how it is > deployed). Do we need to change the deployment process to make use of the file sharing functionality of the cms? (Tbc I agree that we shouldn't make this change now anyway) https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:10: label: "Chrome" On 2018/04/06 12:35:23, juliandoucette wrote: > Are these labels still being used? Good catch, forgot about that! Removed. https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:51: var detectedPlatform = Object.keys(supportedPlatforms).find(hasOwnProperty.bind(bowser)); On 2018/04/06 12:35:23, juliandoucette wrote: > NIT: Line is too long? Done. https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:57: if (detectedPlatform === "maxthon") return; On 2018/04/06 12:35:23, juliandoucette wrote: > NIT: Code style says we should use == :( I can't seem to find where it says that? (Done anyway) https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:67: if (!chrome) return; On 2018/04/06 12:35:23, juliandoucette wrote: > It's possible for chrome to exist without webstore (e.g. when running the test > server or a modified version of chrome). It's also possible for > chrome.webstore.install to fail/error (e.g. because the webstore has invalidated > our domain or the user canceled the install). > > We may decide to handle the success or failure of the install separately (I'm > sure Jeen would be interested). Agreed. > I suggest that we check chrome and chrome.webstore before preventing the default > behaviour of this event. > Agreed, done.
Another round *person hammering emoji* Detail: I've only tested using Chrome and Firefox on OS X. We may push before testing thoroughly as QA should test this entire page across all relevant platforms before publishing. https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:57: if (detectedPlatform === "maxthon") return; On 2018/04/06 13:21:49, ire wrote: > On 2018/04/06 12:35:23, juliandoucette wrote: > > NIT: Code style says we should use == :( > > I can't seem to find where it says that? > > (Done anyway) It's kindof obscure: ABP Coding style > General > 1.1 "Follow the Mozilla Coding Style's general practices and its naming and formatting rules." Mozilla Coding Style > Naming and formatting code > Operators "In JavaScript, == is preferred to ===." https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> I'm not certain that "Install for" should always come before *browser name* when translated. Please confirm with Tamara. I'm not certain that *browser name* should always be separable from "Install for" when translated (e.g. some languages add prefixes and suffixes to nouns). Please confirm with Tamara. --- Suggest: If *browser name* should not always appear after "Install for" then you may use a fixed variable inside "Install for" so that the translator may place it before or after. e.g. "Install for <fix><span id="browser-name">browser</span></fix>". (This would require that you output each browser name separately so that you can insert it into #browser-name appropriately.) Suggest: If *browser name* is not always separable from "install for" then you may place the browser name inside "Install for" to avoid confusion e.g. the translator forgetting that the instance of *browser name* on the homepage should have a prefix or suffix based on the text that appears before or after it in a separate string. e.g. "Install for $browser" for each browser. (This may be a little annoying because we must translate "Install for $browser" and "Agree and Install for $browser" for each browser (presumably) - creating more work for translators entering translations into crowdin and adding more output (strings in our template) to our HTML. (Suggestion #2 also covers suggestion #1. A fixed string is not necessary if the translator may place the browser name themselves.) https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js#... static/js/index.js:25: .find(hasOwnProperty.bind(bowser)); Detail/TOL: I'm a little uncomfortable using `window.hasOwnProperty` (assuming that this function refers to the `window` object's reference to `hasOwnProperty` inherited from `Object.prototype`) because I'm not positive that it will exist across platforms (although I'm fairly confident that it will) and/or not be overwritten (although that is a problem with any object in JavaScript e.g. one can overwrite `Object.prototype.hasOwnProperty` if they choose). I would be more comfortable using `Object.prototype.hasOwnProperty.bind(bowser)` because I think that it refers to the source function and not a more vulnerable reference. I'm probably overthinking this `¯\_(ツ)_/¯` NIT: (Just don't forget that bowser may not be defined :P) https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js#... static/js/index.js:41: if (!chrome || !chrome.webstore) return; This will cause a reference error if `chrome` is not defined (e.g. in Firefox). See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Not_... Suggest: `!chrome instanceof Object || !chrome.webstore instanceof Object` NIT/Suggest: Alternatively, you could test `!chrome instanceof Object` to ignore not-chrome-based browsers and wrap `chrome.webstore.install` in a try/catch block (to catch chrome based browsers without webstore and other obscure errors) and manually redirect to the download page in the catch block. (Nuance: This would catch JS error, not install error. We may handle install error separately like I suggested before.) https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js#... static/js/index.js:47: if (bowser) setupHeroDownloadButton(); I think that this may also cause a reference error if `bowser` is not defined e.g. because the network request for bowser.js failed.
Thanks Julian! New patch up https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29743690/static/js/index.js#... static/js/index.js:57: if (detectedPlatform === "maxthon") return; On 2018/04/06 16:29:16, juliandoucette wrote: > On 2018/04/06 13:21:49, ire wrote: > > On 2018/04/06 12:35:23, juliandoucette wrote: > > > NIT: Code style says we should use == :( > > > > I can't seem to find where it says that? > > > > (Done anyway) > > It's kindof obscure: > > ABP Coding style > General > 1.1 "Follow the Mozilla Coding Style's general > practices and its naming and formatting rules." > > Mozilla Coding Style > Naming and formatting code > Operators "In JavaScript, == > is preferred to ===." > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Acknowledged. https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> On 2018/04/06 16:29:16, juliandoucette wrote: > I'm not certain that "Install for" should always come before *browser name* when > translated. Please confirm with Tamara. > > I'm not certain that *browser name* should always be separable from "Install > for" when translated (e.g. some languages add prefixes and suffixes to nouns). > Please confirm with Tamara. > > --- > > Suggest: If *browser name* should not always appear after "Install for" then you > may use a fixed variable inside "Install for" so that the translator may place > it before or after. > > e.g. "Install for <fix><span id="browser-name">browser</span></fix>". (This > would require that you output each browser name separately so that you can > insert it into #browser-name appropriately.) > > Suggest: If *browser name* is not always separable from "install for" then you > may place the browser name inside "Install for" to avoid confusion e.g. the > translator forgetting that the instance of *browser name* on the homepage should > have a prefix or suffix based on the text that appears before or after it in a > separate string. > > e.g. "Install for $browser" for each browser. (This may be a little annoying > because we must translate "Install for $browser" and "Agree and Install for > $browser" for each browser (presumably) - creating more work for translators > entering translations into crowdin and adding more output (strings in our > template) to our HTML. > > (Suggestion #2 also covers suggestion #1. A fixed string is not necessary if the > translator may place the browser name themselves.) Done. https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js#... static/js/index.js:25: .find(hasOwnProperty.bind(bowser)); On 2018/04/06 16:29:16, juliandoucette wrote: > Detail/TOL: I'm a little uncomfortable using `window.hasOwnProperty` (assuming > that this function refers to the `window` object's reference to `hasOwnProperty` > inherited from `Object.prototype`) because I'm not positive that it will exist > across platforms (although I'm fairly confident that it will) and/or not be > overwritten (although that is a problem with any object in JavaScript e.g. one > can overwrite `Object.prototype.hasOwnProperty` if they choose). I would be more > comfortable using `Object.prototype.hasOwnProperty.bind(bowser)` because I think > that it refers to the source function and not a more vulnerable reference. > > I'm probably overthinking this `¯\_(ツ)_/¯` Done. > NIT: (Just don't forget that bowser may not be defined :P) This function is only called if bowser is defined, so ew don't need to handle those cases within the function https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js#... static/js/index.js:41: if (!chrome || !chrome.webstore) return; On 2018/04/06 16:29:17, juliandoucette wrote: > This will cause a reference error if `chrome` is not defined (e.g. in Firefox). > See > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Not_... > > Suggest: `!chrome instanceof Object || !chrome.webstore instanceof Object` > > NIT/Suggest: Alternatively, you could test `!chrome instanceof Object` to ignore > not-chrome-based browsers and wrap `chrome.webstore.install` in a try/catch > block (to catch chrome based browsers without webstore and other obscure errors) > and manually redirect to the download page in the catch block. (Nuance: This > would catch JS error, not install error. We may handle install error separately > like I suggested before.) Done. https://codereview.adblockplus.org/29727563/diff/29744602/static/js/index.js#... static/js/index.js:47: if (bowser) setupHeroDownloadButton(); On 2018/04/06 16:29:17, juliandoucette wrote: > I think that this may also cause a reference error if `bowser` is not defined > e.g. because the network request for bowser.js failed. Done.
Thanks! The only not-NIT left is a request for clarification. https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> On 2018/04/09 09:24:01, ire wrote: > Done. You didn't say whether or not you determined my second use case and suggestion to relevant? (Here is said use case and suggestion) > I'm not certain that *browser name* should always be separable from "Install > for" when translated (e.g. some languages add prefixes and suffixes to nouns). > Please confirm with Tamara. > Suggest: If *browser name* is not always separable from "install for" then you > may place the browser name inside "Install for" to avoid confusion e.g. the > translator forgetting that the instance of *browser name* on the homepage should > have a prefix or suffix based on the text that appears before or after it in a > separate string. > e.g. "Install for $browser" for each browser. (This may be a little annoying > because we must translate "Install for $browser" and "Agree and Install for > $browser" for each browser (presumably) - creating more work for translators > entering translations into crowdin and adding more output (strings in our > template) to our HTML. https://codereview.adblockplus.org/29727563/diff/29747560/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29747560/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> NIT: I'd prefer to output translations into a single JavaScript object instead of separate templates. ACK: This works and is convenient for this use case. (Meaning that you may ignore my NIT or choose to address it separately.) https://codereview.adblockplus.org/29727563/diff/29747560/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29747560/static/js/index.js#... static/js/index.js:25: .find(bowser.hasOwnProperty.bind(bowser)) NIT: You removed the semicolon.
On 2018/04/11 20:29:45, juliandoucette wrote: > See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/53#note_67969303 Ack. Jeen/Tiago haven't answered, so I assume we continue as is until we decide otherwise
Thanks Julian! New patch set up. https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> On 2018/04/09 13:01:02, juliandoucette wrote: > On 2018/04/09 09:24:01, ire wrote: > > Done. > > You didn't say whether or not you determined my second use case and suggestion > to relevant? > > (Here is said use case and suggestion) > > > I'm not certain that *browser name* should always be separable from "Install > > for" when translated (e.g. some languages add prefixes and suffixes to nouns). > > Please confirm with Tamara. > > > Suggest: If *browser name* is not always separable from "install for" then you > > may place the browser name inside "Install for" to avoid confusion e.g. the > > translator forgetting that the instance of *browser name* on the homepage > should > > have a prefix or suffix based on the text that appears before or after it in a > > separate string. > > > e.g. "Install for $browser" for each browser. (This may be a little annoying > > because we must translate "Install for $browser" and "Agree and Install for > > $browser" for each browser (presumably) - creating more work for translators > > entering translations into crowdin and adding more output (strings in our > > template) to our HTML. I didn't think it would be an issue since we would be swapping out the "browser" string. They way I see it, even if the language changes things or compounds the words, swapping out "browser" shouldn't change anything as it's just the name. However, I have messaged Tamara to confirm, she hasn't gotten back to me yet. https://codereview.adblockplus.org/29727563/diff/29747560/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29747560/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> On 2018/04/09 13:01:02, juliandoucette wrote: > NIT: I'd prefer to output translations into a single JavaScript object instead > of separate templates. > > ACK: This works and is convenient for this use case. (Meaning that you may > ignore my NIT or choose to address it separately.) Ack, I think we can handle this separately then. https://codereview.adblockplus.org/29727563/diff/29747560/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29747560/static/js/index.js#... static/js/index.js:25: .find(bowser.hasOwnProperty.bind(bowser)) On 2018/04/09 13:01:02, juliandoucette wrote: > NIT: You removed the semicolon. Done.
Response from Tamara below. https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29744602/includes/index.tmpl... includes/index.tmpl:15: <script id="hero-download-button-template" type="text/template"> On 2018/04/12 08:05:46, ire wrote: > I didn't think it would be an issue since we would be swapping out the "browser" > string. They way I see it, even if the language changes things or compounds the > words, swapping out "browser" shouldn't change anything as it's just the name. > However, I have messaged Tamara to confirm, she hasn't gotten back to me yet. From Tamara in the email thread "RE: translation of "[Agree and] Install for ${browser}" from abp.org sync meeting": I do agree that use case 2 would be better. Hopefully both easier to translate and to implement. Nothing else to add from my side. :) Detail: Here was my explanation of the two use cases to her: > My explanation in the code review may not be clear (sorry). > > In use case 1 I am talking about translating "Agree and Install for <fix>browser</fix>" and each browser name "chrome", "firefox", ... separately. And then inserting the browser name into the place of <fix>browser</fix> dynamically. > > In use case 2 I am talking about translating this string for each browser separately e.g. "Agree and Install for Chrome", "Agree and Install for Firefox"... > > Ire seems to prefer use case 1 (I do too at first glance). But I think use case 2 may be more appropriate for you & other translators because it may be more straightforward and thus less vulnerable to misinterpretation and error. > > Do you agree with my conclusion about use case 2? And/or do you have anything to add?
> From Tamara in the email thread "RE: translation of "[Agree and] Install for
${browser}" from abp.org sync meeting":
Yes I saw it, thanks. New patch set up
Quick note: You permanently uncovered the IE only message in your last change.
NITs + IE text issue. The rest looks good :). https://codereview.adblockplus.org/29727563/diff/29753582/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29753582/includes/index.tmpl... includes/index.tmpl:16: <script id="hero-download-button-template-{{ browser.id }}" type="text/template"> NIT/Suggest: id="download-label-$browser" (it's actually the "label" / "innerText" in this template and this id is very long). https://codereview.adblockplus.org/29727563/diff/29753582/static/css/index.css File static/css/index.css (left): https://codereview.adblockplus.org/29727563/diff/29753582/static/css/index.cs... static/css/index.css:14: .show-on-msie { display: none; } RE: Last published quick note. It looks like you have to re-enter this. https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js#... static/js/index.js:35: heroDownloadButton.innerHTML = document NIT/Suggest: innerText (we don't support IE 8 anymore and it's ~more secure?) https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js#... static/js/index.js:44: try TOL: Why are curly braces required for try/catch statements :/?
Thanks, new patch set https://codereview.adblockplus.org/29727563/diff/29753582/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29727563/diff/29753582/includes/index.tmpl... includes/index.tmpl:16: <script id="hero-download-button-template-{{ browser.id }}" type="text/template"> On 2018/04/16 15:44:40, juliandoucette wrote: > NIT/Suggest: id="download-label-$browser" (it's actually the "label" / > "innerText" in this template and this id is very long). Done. https://codereview.adblockplus.org/29727563/diff/29753582/static/css/index.css File static/css/index.css (left): https://codereview.adblockplus.org/29727563/diff/29753582/static/css/index.cs... static/css/index.css:14: .show-on-msie { display: none; } On 2018/04/16 15:44:40, juliandoucette wrote: > RE: Last published quick note. It looks like you have to re-enter this. Thanks! Done. https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js#... static/js/index.js:35: heroDownloadButton.innerHTML = document On 2018/04/16 15:44:41, juliandoucette wrote: > NIT/Suggest: innerText (we don't support IE 8 anymore and it's ~more secure?) Changed to textContent which seems to be better for performance (https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent, https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent) https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js#... static/js/index.js:44: try On 2018/04/16 15:44:41, juliandoucette wrote: > TOL: Why are curly braces required for try/catch statements :/? I know right?!?! Maybe there's a good reason I don't know about
LGTM https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29727563/diff/29753582/static/js/index.js#... static/js/index.js:35: heroDownloadButton.innerHTML = document On 2018/04/16 16:00:59, ire wrote: > On 2018/04/16 15:44:41, juliandoucette wrote: > > NIT/Suggest: innerText (we don't support IE 8 anymore and it's ~more secure?) > > Changed to textContent which seems to be better for performance > > (https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent, > https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent) Thanks! (Facepalm: I meant to say textContent. My unconsciously wrote the last thing I read by mistake.) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
