Fixes #35 - Progressively enhance install button with appropriate links and text

static/js/index.js

 "use strict";

[juliandoucette, 2018/04/04 10:18:23]

Why is this necessary?

[ire, 2018/04/05 14:32:43]

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)

[juliandoucette, 2018/04/06 12:35:22]

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.)

[ire, 2018/04/06 13:21:48]

Acknowledged.

 
 (function()
 {
   var supportedPlatforms = {
+
     // Desktop browsers

[juliandoucette, 2018/04/04 10:18:23]

NIT: Please put one blank line above to make this more noticable

[ire, 2018/04/05 14:32:44]

Done.

-    "chrome": {
-      installer: "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb",
-      label: "Chrome"
-    },
-    "opera": {
-      installer: "https://addons.opera.com/extensions/details/opera-adblock/?display=en-US",
-      label: "Opera"
-    },
-    "yandexbrowser": {
-      installer: "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb",
-      label: "Yandex Browser"
-    },
-    "msie": {
-      installer: "https://update.adblockplus.org/latest/adblockplusie.exe",
-      label: "Internet Explorer"
-    },
-    "msedge": {
-      installer: "https://www.microsoft.com/store/p/adblock-plus/9nblggh4r9nz",
-      label: "Micrsoft Edge"
-    },
-    "firefox": {
-      installer: "https://update.adblockplus.org/latest/adblockplusfirefox.xpi",
-      label: "Firefox"
-    },
-    "safari": {
-      installer: "https://update.adblockplus.org/latest/adblockplussafari.safariextz",
-      label: "Safari"
-    },
+    "chrome": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb",
+    "opera": "https://addons.opera.com/extensions/details/opera-adblock/?display=en-US",
+    "yandexbrowser": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb",
+    "msie": "https://update.adblockplus.org/latest/adblockplusie.exe",
+    "msedge": "https://www.microsoft.com/store/p/adblock-plus/9nblggh4r9nz",
+    "firefox": "https://update.adblockplus.org/latest/adblockplusfirefox.xpi",
+    "safari": "https://update.adblockplus.org/latest/adblockplussafari.safariextz",
     "maxthon": "",
 
     // Mobile platforms
-    "ios": {
-      installer: "https://eyeo.to/adblockbrowser/ios/abp-website",
-      label: "iOS",
-    },
-    "android": {
-      installer: "https://eyeo.to/adblockbrowser/android/abp-website",
-      label: "Android"
-    }
+    "ios": "https://eyeo.to/adblockbrowser/ios/abp-website",
+    "android": "https://eyeo.to/adblockbrowser/android/abp-website"
   };
 
   function setupHeroDownloadButton()
   {
-    var heroDownloadButton = document.getElementById("hero-download-button");
-    var detectedPlatform;
-
-    for (var key in supportedPlatforms) if (bowser[key]) detectedPlatform = key;

[juliandoucette, 2018/04/04 10:18:23]

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.

[ire, 2018/04/05 14:32:43]

I'll go with your implementation. 

Done.

+    var detectedPlatform = Object.keys(supportedPlatforms)
+      .find(bowser.hasOwnProperty.bind(bowser));
 
     if (!detectedPlatform) return;
 
     document.body.classList.add(detectedPlatform);
 
-    if (detectedPlatform === "maxthon") return;
-    
-    heroDownloadButton.href = supportedPlatforms[detectedPlatform].installer;
+    if (detectedPlatform == "maxthon") return;
+
+    var heroDownloadButton = document.getElementById("hero-download-button");
+    heroDownloadButton.href = supportedPlatforms[detectedPlatform];
     heroDownloadButton.textContent = document
-      .getElementById("hero-download-button-template")
-      .innerHTML
-      .replace("browser", supportedPlatforms[detectedPlatform].label);

[juliandoucette, 2018/04/04 10:18:23]

"browser" is not fixed in the include. Therefore, you cannot rely on "browser"
existing in the string.

[juliandoucette, 2018/04/04 10:18:23]

I think the browser name itself is translated in some languages. Please check
with C&T if you haven't already.

[ire, 2018/04/05 14:32:43]

Done.

[ire, 2018/04/05 14:32:43]

Done.

+      .getElementById("download-label-" + detectedPlatform)
+      .textContent;
     
     heroDownloadButton.addEventListener("click", function(event)

[juliandoucette, 2018/04/04 10:18:23]

I think chrome.webstore.install works on other chrome based browsers?

[ire, 2018/04/05 14:32:43]

You're right. I switch to checking for the "chrome" variable itself

     {
-      if (detectedPlatform !== "chrome") return;
+      if (typeof chrome == "undefined") return;
       event.preventDefault();
-      chrome.webstore.install();
+
+      try
+      {
+        chrome.webstore.install();
+      }
+      catch(error)
+      {
+        window.location = "/" + this.hreflang + "/download";
+      }
     });
   }
 
-  if (bowser) setupHeroDownloadButton();
+  if (typeof bowser != "undefined") setupHeroDownloadButton();
 
 }());