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

static/js/main.js

 (function(){
   document.addEventListener("DOMContentLoaded", function()
   {
 
     /**************************************************************************
      * General
      **************************************************************************/
 
     // Change html class name from "no-js" to "js"
     document.documentElement.className = "js";
@@ skipping 181 matching lines @@
       
       this.toggleSection(event.target);
     }
 
     var productTopicsAccordion = document.getElementById('product-topics-accordion');
     if (productTopicsAccordion)
     {
       new Accordion(productTopicsAccordion);
     }
 
+    /**************************************************************************
+     * BrowserSelect
+     **************************************************************************/
+
+    function BrowserSelect(select)
+    {
+      this.browserSelect = select;

[juliandoucette, 2017/10/24 12:12:28]

NIT: You are repeating yourself by prefacing these names with "browser" e.g.
browserSelect.browserSelect browserSelect.browserCustomSelect vs
browserSelect.select browserSelect.customSelect

[ire, 2017/10/27 10:22:57]

You're right. Done.

+      this.browserCustomSelect = new CustomSelect(this.browserSelect);

[juliandoucette, 2017/10/24 12:12:28]

NIT/Suggest: Looking at this again, I think that the browser select *is*, not
*has*, a CustomSelect. And it should extend the CustomSelect prototype for that
reason.

[ire, 2017/10/27 10:22:56]

Done.

+      
+      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);
+      
+      this.browserSelect
+        .addEventListener("keydown", this._onClickOrKeyDown.bind(this), false);
+
+      var storedBrowser = localStorage.getItem(this.BROWSER_STORAGE_KEY);

[juliandoucette, 2017/10/24 12:12:28]

NIT/Suggest: Place this above event listeners with other definitions 

[ire, 2017/10/27 10:22:56]

I think I prefer this being close to where it's used in this case.

+      if (storedBrowser)

[juliandoucette, 2017/10/24 12:12:29]

NIT/Suggest: Our coding style permits braceless one line if/else blocks.

[ire, 2017/10/27 10:22:58]

Done.

+      {
+        this.selectOption(storedBrowser);
+      }
+      else
+      {
+        this.detectBrowser();
+      }
+    }

[juliandoucette, 2017/10/24 12:12:28]

NIT: Missing ";" (the same applies elsewhere)

[ire, 2017/10/27 10:22:57]

The ";" isn't used for the closing brace of functions (function name() {}). 

But this applies elsewhere. Done.

[juliandoucette, 2017/10/31 13:19:57]

*facepalm* (because I commented on the only place where my comment didn't apply)

[ire, 2017/11/01 12:36:47]

Haha :D

+
+    BrowserSelect.prototype.detectBrowser = function()
+    {
+      localStorage.setItem(this.BROWSER_AUTODETECTED_STORAGE_KEY, "true");
+
+      var browser;
+      if (bowser.chrome) browser = "chrome";

[juliandoucette, 2017/10/24 12:12:29]

NIT/Suggest:

this.supportedBrowsers = ["chorme", "firefox", ...]; // (should probably go in
the constructor)

for (var supportedBrowser in this.supportedBrowsers)
  if (browser[supportedBrowser])
    return this.selectOption(supportedBrowser);

localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
this.selectOption(this.DEFAULT_BROWSER);

[ire, 2017/10/27 10:22:57]

I need to check for the specific browser name to set it as the variable name we
want. This is why I didn't use an array.

The other option would be for us to use the same browser variable names as
bowser does.

[juliandoucette, 2017/10/31 13:19:58]

I did so by setting the variable `supportedBrowser` in my example. I don't see
any other relevant usages of your overridden `browser` variable?

[ire, 2017/11/01 12:36:47]

Ooo I see! I don't think I read your example properly, sorry about that.

The only other thing is that we aren't using the same browser names as bowser
does. So we can't set the browser to be "yandexBrowser" because we would want it
to be "yandex". So our supportedBrowsers array will actually have to container
two variables (bowser name and our name). It still think your method would be
cleaner, but may not necessarily be shorter. What do you think?

[juliandoucette, 2017/11/01 12:49:21]

Why don't we use their names instead?

[ire, 2017/11/01 17:20:11]

We could. Done.

+      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";
+      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

[juliandoucette, 2017/10/24 12:12:29]

NIT: I don't think that this comment is necessary?

[ire, 2017/10/27 10:22:56]

Done.

+      localStorage.setItem(this.BROWSER_STORAGE_KEY, browser);
+
+      // Change body class
+      var bodyClassList = Array.prototype.slice.call(document.body.classList);

[juliandoucette, 2017/10/24 12:12:27]

document.body.classList has length. You don't need to make it into an Array. If
you don't, then you could use this variable below to bodyClassList.add &
bodyClassList.remove.

[ire, 2017/10/27 10:22:57]

I can't do it that way because when a class is removed, I can't rely on the
index again to determine where it is. I have to create a separate array that
doesn't change it's length.

[juliandoucette, 2017/10/31 13:19:57]

Acknowledged. Doh.

+      for (var i = 0; i < bodyClassList.length; i++)
+      {
+        if (bodyClassList[i].indexOf('ua-') > -1)
+        {
+          document.body.classList.remove(bodyClassList[i]);
+        }
+      }
+      document.body.classList.add("ua-" + browser);
+
+      // Check selected option
+      var selectedItem = this.browserSelect

[juliandoucette, 2017/10/24 12:12:29]

I think that we usually open the brace on the same line in this case (or the
next if we are chaining). The same applies below.

[ire, 2017/10/27 10:22:58]

I moved the .querySelector line because it would be longer than 80 characters.

[juliandoucette, 2017/10/31 13:19:58]

I know. My comment still applies? We usually open the brace instead. And if we
don't then we start one indentation after the start of the line. This isn't
spelled out in our coding style; unfortunately. And it seems like a common case.
Maybe we can find or add something about it.

[juliandoucette, 2017/10/31 13:24:02]

Can you advise us on this kzar?

[kzar, 2017/10/31 14:22:31]

IIRC we don't have a ESLint rule for this case, so it's up to you as long as the
code lints. I do remember seeing Wladimir do something similar recently[1].

P.S. if it helps I found a bunch of lines starting with a dot operator[2], have
a look through those if you want to have a look at how it's commonly done.

[1]
https://github.com/adblockplus/adblockplusui/commit/f5403db7476aadd1c7519485a...
[2] https://gist.github.com/kzar/8d3e7c6ff8c7f02e776b87ffd9f63f14

[ire, 2017/11/01 12:36:48]

Thanks kzar!

+                             .querySelector("[data-value='" + browser + "']");
+      selectedItem.setAttribute("aria-checked", "true");
+
+      // Set selected option
+      var selectedOption = document.createElement("li");

[juliandoucette, 2017/10/24 12:12:28]

You don't seem to be using this element (just manipulating it's innerHTML which
could be stored inside a variable instead of an element).

[ire, 2017/10/27 10:22:57]

You're right, don't notice that :p

Done.

+      selectedOption.innerHTML = selectedItem.innerHTML;
+
+      if (localStorage.getItem(this.BROWSER_AUTODETECTED_STORAGE_KEY))
+      {
+        var autodetected = document
+                              .getElementById('browser-select-autodetected')
+                              .innerHTML;
+        selectedOption.innerHTML += "<span class='muted'>(" + autodetected + ")</span>";
+      }
+
+      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;
+
+      localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
+
+      // Uncheck previously checked option
+      this.browserSelect
+          .querySelector("[aria-checked='true']")
+          .setAttribute("aria-checked", "false");
+
+      this.selectOption(event.target.getAttribute("data-value"));
+
+      // Close Select
+      this.browserCustomSelect.close();
+    }
+
+    var browserSelect = document.getElementById("browser-select");
+    if (browserSelect)
+    {
+      new BrowserSelect(browserSelect);
+    }
+
   }, false);
 }());