Issue 5692 - Create Browser Selector with Browser Detection Component for help.eyeo.com

static/js/main.js

Index: static/js/main.js
===================================================================
--- a/static/js/main.js
+++ b/static/js/main.js
@@ -194,10 +194,111 @@
     }
 
     var productTopicsAccordion = document.getElementById('product-topics-accordion');
     if (productTopicsAccordion)
     {
       new Accordion(productTopicsAccordion);
     }
 
+    /**************************************************************************
+     * BrowserSelect
+     **************************************************************************/
+
+    function BrowserSelect(select)

[juliandoucette, 2017/10/13 13:39:56]

Note: You could extend CustomSelect's prototype (e.g. BrowserSelect.prototype =
extend({}, CustomSelect.prototype)) and apply CustomSelect (e.g.
CustomSelect.apply(this, [select]) and use this.select instead of
this.browserSelect.

What you did here is fine too.

[ire, 2017/10/17 15:03:33]

Cool. I'll keep what i did here then.

+    {
+      this.browserSelect = select;
+      this.browserCustomSelect = new CustomSelect(this.browserSelect);
+      
+      this.DEFAULT_BROWSER = "chrome";
+      this.BROWSER_STORAGE_KEY = "BROWSER";
+      this.BROWSER_AUTODETECTED_STORAGE_KEY = "BROWSER_AUTODETECTED";
+      
+      this.browserSelect
+        .addEventListener("click", this._onClickOrKeyDown.bind(this), false);
+      

[juliandoucette, 2017/10/13 13:39:56]

NIT: You should be able to click-away too (click elsewhere to close the
dropdown)?

[ire, 2017/10/17 15:03:34]

Yes, I will fix this in a separate issue

https://issues.adblockplus.org/ticket/5868#ticket

[juliandoucette, 2017/10/18 15:04:10]

Acknowledged.

+      this.browserSelect
+        .addEventListener("keydown", this._onClickOrKeyDown.bind(this), false);
+
+      var storedBrowser = localStorage.getItem(this.BROWSER_STORAGE_KEY);
+      if (storedBrowser)
+      {
+        this.selectOption(storedBrowser);
+      }
+      else
+      {
+        this.detectBrowser();
+      }
+    }
+
+    BrowserSelect.prototype.detectBrowser = function()
+    {
+      localStorage.setItem(this.BROWSER_AUTODETECTED_STORAGE_KEY, "true");
+
+      var browser;
+      if (bowser.chrome) browser = "chrome";

[juliandoucette, 2017/10/13 13:39:55]

Wait, what? How can browser have properties if it was just defined as undefined?

[ire, 2017/10/17 15:03:34]

You can override previously defined (or undefined) properties in this way

[juliandoucette, 2017/10/18 15:04:10]

Can you elaborate / explain when/how this variable is not undefined?

[ire, 2017/10/20 13:40:46]

Sure!

I create a pen to illustrate this -
https://codepen.io/ire/pen/MExoQM?editors=0011

Basically, variables created with var/let can have their values updated in this
way. You can initialise a variable with one value, then update it later on.
Then, depending when the variable reference is used, the value can be different.

+      else if (bowser.opera) browser = "opera";
+      else if (bowser.coast) browser = "opera";
+      else if (bowser.samsungBrowser) browser = "samsung";
+      else if (bowser.yandexbrowser) browser = "yandex";
+      else if (bowser.maxthon) browser = "maxthon";
+      else if (bowser.msie) browser = "ie";
+      else if (bowser.msedge) browser = "edge";
+      else if (bowser.firefox) browser = "firefox";
+      else if (bowser.ios) browser = "ios";
+      else if (bowser.safari) browser = "safari";

[juliandoucette, 2017/10/18 15:04:10]

Why check each browser property and not *just* browser.name?

[ire, 2017/10/20 13:40:46]

I felt it wouldn't be as reliable/steady as this. I couldn't find a set list of
the browser names without having to go into the bowser.js code. 

+      else
+      {
+        localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
+        browser = this.DEFAULT_BROWSER;
+      }
+
+      this.selectOption(browser);  
+    }
+
+    BrowserSelect.prototype.selectOption = function(browser)
+    {
+      // Save value to Local Storage
+      localStorage.setItem(this.BROWSER_STORAGE_KEY, browser);
+
+      // Change body class
+      document.body.className = "ua-"+browser;

[juliandoucette, 2017/10/13 13:39:55]

What if there are not related classes attached to the body later?

[juliandoucette, 2017/10/13 13:41:36]

unrelated*

[ire, 2017/10/17 15:03:34]

Done.

+
+      // Set selected option
+      var selectedOption = document.createElement("li");
+      selectedOption.innerHTML = this
+                                  .browserSelect

[juliandoucette, 2017/10/13 13:39:55]

NIT: spaces around +?

[ire, 2017/10/17 15:03:34]

I thought we don't put spaces within the ()?

[juliandoucette, 2017/10/18 15:04:11]

We don't put spaces after the opening brace and before the closing brace AFAIK
(I can't find a reference in our code-style).

(1 + 1) //good
( 1 + 1 ) //bad

[ire, 2017/10/20 13:40:48]

Alright, makes sense. Done.

+                                  .querySelector("[data-value='"+browser+"']")
+                                  .innerHTML;
+
+      if (localStorage.getItem(this.BROWSER_AUTODETECTED_STORAGE_KEY))
+      {
+        selectedOption.innerHTML += "<span class='muted'>(autodetected)</span>";

[juliandoucette, 2017/10/13 13:39:55]

This isn't translated. I suggest translating it in the HTML and show/hiding or
add/removing it.

[ire, 2017/10/17 15:03:34]

I place it in a template so I wouldn't have to have it in the HTML for all items

[juliandoucette, 2017/10/18 15:04:10]

You mean you will place it in a template? Or something else?

[ire, 2017/10/20 13:40:48]

Sorry I meant to say "I will place it in a template" (as I have done)

+      }
+
+      this.browserSelect
+        .querySelector(".custom-select-selected")
+        .innerHTML = selectedOption.innerHTML;
+    }
+
+    BrowserSelect.prototype._onClickOrKeyDown = function(event)
+    {
+      if (!event.target.classList.contains("custom-select-option")) return;
+
+      var IS_ENTER_KEY = event.key == "Enter" || event.keyCode == 13;
+      if (event.keyCode && !IS_ENTER_KEY) return;

[juliandoucette, 2017/10/13 13:39:56]

Why check event.keyCode here?

[ire, 2017/10/17 15:03:33]

Because this function is also relevant for clicks. I'm checking here to return
only if it was a keyDown event AND the key wasn't the enter key. 

[juliandoucette, 2017/10/18 15:04:11]

Acknowledged. I was confused.

+
+      localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
+
+      this.selectOption(event.target.getAttribute("data-value"));
+
+      // Close Select
+      this.browserCustomSelect.close();
+    }
+
+    var browserSelect = document.getElementById("browser-select");
+    if (browserSelect)
+    {
+      new BrowserSelect(browserSelect);
+    }
+
   }, false);
 }());