Issue 6003 - Better handling of no-content-for-platform-message on help.eyeo.com

static/js/main.js

Index: static/js/main.js
===================================================================
--- a/static/js/main.js
+++ b/static/js/main.js
@@ -230,38 +230,57 @@
     /**************************************************************************
      * BrowserSelect
      **************************************************************************/
 
     function BrowserSelect(select)
     {
       this.select = select;
       CustomSelect.apply(this, [this.select]);
+      this.noContentMessage = document.getElementById("no-content-for-platform-message");
       
       this.BROWSER_STORAGE_KEY = "BROWSER";
       this.BROWSER_AUTODETECTED_STORAGE_KEY = "BROWSER_AUTODETECTED";
       this.SUPPORTED_BROWSERS = ["chrome", "opera", "samsungBrowser", 
                                  "yandexbrowser", "maxthon", "msie", 
                                  "msedge", "firefox", "ios", "safari"];
       this.DEFAULT_BROWSER = "chrome";
       
+      this.setCurrentArticleSupportedBrowsers();
+      
       this.select
         .addEventListener("click", this._onClickOrKeyDown.bind(this), false);
       
       this.select
         .addEventListener("keydown", this._onClickOrKeyDown.bind(this), false);
 
+      this.noContentMessage
+        .addEventListener("click", this._onClickNoContentMessage.bind(this), false);
+
       var storedBrowser = localStorage.getItem(this.BROWSER_STORAGE_KEY);
       if (storedBrowser) this.selectOption(storedBrowser);
       else this.detectBrowser();
     }
 
     BrowserSelect.prototype = Object.create(CustomSelect.prototype);
     BrowserSelect.prototype.constructor = BrowserSelect;
 
+    BrowserSelect.prototype.setCurrentArticleSupportedBrowsers = function()
+    {
+      for (var i = 0; i < this.SUPPORTED_BROWSERS.length; i++)
+      {
+        var supportedBrowser = this.SUPPORTED_BROWSERS[i];

[juliandoucette, 2017/12/11 15:15:28]

NIT: Despite "Declare local variables as near to their use as possible." [1] I
would argue "JavaScript does not have block scope, so defining variables in
blocks can confuse programmers who are experienced with other C family
languages. Define all variables at the top of the function." [2] (Acknowledging
that ES6 *does* have block scope) (Acknowledging that this may be an aging
generational preference.)

[1]
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

[2] http://crockford.com/javascript/#variable%20declarations

[ire, 2017/12/12 10:21:13]

I'm not sure I understand where else this variable would go? It is dependent on
the variable i in the line before and used in the line immediately after.

[juliandoucette, 2017/12/12 12:30:31]

Traditionally...

```
var name;

for (...) {
  name = names[i];
  ...
```

But you could also...

```
for (var name in browsers) {
  ...
```

Or...

```
browsers.forEach(function(name){
  ...
```

[ire, 2017/12/13 10:30:18]

Thanks for the suggestions. Responses:

On 2017/12/12 12:30:31, juliandoucette wrote:
I don't see the purpose of separating the variable declaration and assignment
(more lines?). 
When using for..in loops with Arrays, the variable is actually the key, so it
would actually be `for (var index in browsers)`. Plus, I read that using for..in
loops with Arrays isn't advisable since it doesn't always happen in order
(although I realise that's not important in this case) 

Source:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...
This would probably have been the best way to do it. I have generally stayed
away from forEach because of IE8 but since we aren't "officially" supporting IE8
then I guess I could have used it. 

[juliandoucette, 2017/12/13 14:57:47]

To avoid confusion about the scope of the variable.
Ack.
Ack/Agreed.

+        if (!document.querySelector(".platform-"+supportedBrowser))

[juliandoucette, 2017/12/11 15:15:28]

NIT/TOL: querySelector supports selector groups e.g. ".platform-chrome,
.platform-firefox". Therefore, it may be more efficient to explode
SUPPORTED_BROWSERS into a selector and perform one query than performing each
query individually.

e.g. querySelector(browsers.join('-platform, ')) //works with -platform suffix,
not prefix

Detail: The performance impact will be negligible in this case. Therefore this
is more of a SuperNIT programming style issue. And I chose to point it out
instead of ignoring it in-case you like this solution personally or learn
something new e.g. that selector groups are a thing.

[ire, 2017/12/12 10:21:13]

Thanks! I did know of selector groups but didn't think of it in this case.

I don't think this particular method could be used here since I am checking for
the absence of a particular selector, so I may still need to perform the loop
anyway. But thanks for pointing this out, it's at least brought this method back
into my consciousness for another use. 

[juliandoucette, 2017/12/12 12:30:31]

If a selector in a selector group matches no results then no results from that
selector are added to the resulting nodelist. As a result, when you loop *just*
the matching results, you are performing fewer iterations (where you in turn
perform one fewer condition and one fewwer DOM query).

[ire, 2017/12/13 10:30:18]

Ah I see! I think your solution is preferable. I may go back and change it but I
don't think it is currently worth having to change all the `platform-${browser}`
selectors to work the other way around. 

[juliandoucette, 2017/12/13 14:57:47]

Agreed.

+        {
+          this.noContentMessage.querySelector("[data-value='" + supportedBrowser + "']")
+              .setAttribute("hidden", "true");
+        }
+      }
+    };
+
     BrowserSelect.prototype.detectBrowser = function()
     {
       for (var i = 0; i < this.SUPPORTED_BROWSERS.length; i++)
       {
         var supportedBrowser = this.SUPPORTED_BROWSERS[i];
         if (bowser[supportedBrowser])
         {
           localStorage.setItem(this.BROWSER_AUTODETECTED_STORAGE_KEY, "true");
@@ -301,36 +320,27 @@
                               .getElementById("browser-select-autodetected")
                               .innerHTML;
         selectedOption += "<span class='muted'>(" + autodetected + ")</span>";
       }
 
       this.select
           .querySelector(".custom-select-selected")
           .innerHTML = selectedOption;
-      
-      if (!document.querySelector(".platform-" + browser))
+
+      if (document.querySelector(".platform-" + browser))
       {
-        this.handleNoContentForBrowser(browser);
+        this.noContentMessage.setAttribute("hidden", "true");
+      }
+      else
+      {
+        this.noContentMessage.removeAttribute("hidden");
       }
     };
 
-    BrowserSelect.prototype.handleNoContentForBrowser = function(browser)
-    {
-      var section = document.createElement("section");
-      section.classList.add("platform-" + browser);
-      section.innerHTML = document
-                            .getElementById("no-content-for-platform-message")
-                            .innerHTML;
-
-      document
-        .querySelector(".article-body")
-        .insertAdjacentElement("afterbegin", section);
-    }
-
     BrowserSelect.prototype._onClickOrKeyDown = function(event)
     {
       var option = event.target.closest(".custom-select-option");
       if (!option) return;
 
       var IS_ENTER_KEY = event.key == "Enter" || event.keyCode == 13;
       if (event.keyCode && !IS_ENTER_KEY) return;
 
@@ -341,16 +351,24 @@
           .querySelector("[aria-checked='true']")
           .setAttribute("aria-checked", "false");
 
       this.selectOption(option.getAttribute("data-value"));
 
       this.close();
     };
 
+    BrowserSelect.prototype._onClickNoContentMessage = function(event)
+    {
+      if (event.target.tagName != "BUTTON") return;
+
+      var browser = event.target.parentElement.getAttribute("data-value");

[juliandoucette, 2017/12/11 15:15:28]

NIT: Unnecessary variable.

this.selectOption(
  event.target.parentElement.getAttribute("data-value")
);

[ire, 2017/12/12 10:21:13]

Done.

+      this.selectOption(browser);
+    };
+
     var browserSelect = document.getElementById("browser-select");
     if (browserSelect)
     {
       new BrowserSelect(browserSelect);
     }
 
   }, false);
 }());