No Issue - Implemented first run page

firstRun.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals checkShareResource, getDocLink, openSharePopup, setLinks, E */

[juliandoucette, 2017/10/21 16:18:43]

@greiner, @saroyanm

"[Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated
because of its detrimental effects to the end user's experience. For more help,
check https://xhr.spec.whatwg.org/."

Is this something that we should worry about now?

[Thomas Greiner, 2017/10/23 12:01:55]

No, this issue only applies to the development environment because we have to
load translations synchronously to mock `chrome.i18n.getMessage()`. Obviously,
we still need to change our mock implementation at some point to avoid breakage
but it's not relevant for running the UI within the extension.

[Thomas Greiner, 2018/02/16 16:19:14]

Detail: We no longer use `checkShareResource()` and `openSharePopup()` so no
need to list them here. We can even remove those two functions from commons.js
now since this was the only page that was using them.

 
 "use strict";
 
 (function()
 {
+  function openFilters()
+  {
+    browser.runtime.sendMessage({type: "app.open", what: "options"});
+  }
+
+  function navigationClick(event)
+  {
+    document.getElementById("navbar-menu").classList.toggle("visible");

[Thomas Greiner, 2018/02/16 16:19:12]

Detail: I don't mind using either `E()` or `document.getElementById()` but at
least we should be consistent by using either the one or the other but not both.

+  }
+
+  function initMenu()
+  {
+    document.getElementById("navbar-menu-toggle").addEventListener("click", navigationClick, false);

[Thomas Greiner, 2018/02/16 16:19:11]

Detail: The third function argument parameter is redundant. We used to pass it
in the past which is why it's still used for "DOMContentLoaded" but we have
since stopped doing that.

[Thomas Greiner, 2018/02/16 16:19:14]

Coding style: "Line length: 80 characters or less"
Also applies to line 50.

Note that you can now automatically detect and fix coding style issues by
running `npm run lint` (see https://github.com/adblockplus/adblockplusui#linting
as well as https://github.com/adblockplus/adblockplusui#installing-dependencies
for more info on that).

+  }
+
   function onDOMLoaded()
   {
-    function initLanguageSelection()
-    {
-      const locale = document.getElementById("navbar-locale-selected");
-      // skip if page does not have language selection (EG: blog)
-      if (!locale)
-        return;
-
-      locale.onclick = function()
-      {
-        toggleClass(document.getElementById("navbar-locale-menu"), "visible");
-      };
-    }
-
-    function navigationClick(event)
-    {
-      toggleClass(document.getElementById("navbar-menu"), "visible");
-    }
-
-    function initMenu()
-    {
-      document.getElementById("navbar-menu-toggle").onclick = navigationClick;
-    }
-
-    function toggleClass(element, className)

[Thomas Greiner, 2017/10/23 13:17:43]

We've already been using `Element.classList` in other pages so please use it
instead of introducing those utility functions for handling CSS classes.

[juliandoucette, 2017/10/23 13:27:35]

He copied this from abp.org main.js. It's unclear to me whether we should
re-write this for abpui or just separate it. What do you think?

[juliandoucette, 2017/10/23 13:29:58]

(On that note, we could polyfill classList and refactor main.js in abp.org.)

[Thomas Greiner, 2017/10/23 16:45:49]

From what I see, if we remove the language selection code, there'd only be one
instance left where we actually use it. So I'd say there's little value in
keeping it just for the sake of consistency with Websites.
True, good idea.

[juliandoucette, 2017/10/24 14:22:18]

See https://codereview.adblockplus.org/29587659

[martin, 2017/10/26 10:33:16]

Done.

-    {
-      if (hasClass(element, className))
-        removeClass(element, className);
-      else
-        addClass(element, className);
-    }
-
-    function hasClass(element, className)
-    {
-      return !!element.className.match("\\b" + escapeRegExp(className) + "\\b");
-    }
-
-    function addClass(element, className)
-    {
-      if (hasClass(element, className))
-        return;
-
-      if (element.className.length)
-        element.className += " ";
-      element.className += className;
-    }
-
-    function removeClass(element, className)
-    {
-      const regExp = new RegExp("\\s*\\b" + escapeRegExp(className) + "\\b\\s*");
-      element.className = element.className.replace(regExp, "");
-    }
-
-    function escapeRegExp(string)
-    {
-      return string.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, "\\$&");
-    }
-
-    initLanguageSelection();
-    initMenu();
-
     const optionsTrigger = E("options-trigger");

[Thomas Greiner, 2018/02/16 16:19:13]

Coding style: "Use const for constant expressions that could as well have been
inlined (e.g. static parameters, imported modules, etc.)."

In this case the value of `optionsTrigger` is not a constant expression but a
function call.

 
     optionsTrigger.addEventListener("click", (e) =>
     {
       e.preventDefault();
       openFilters();
     });
 
     setLinks("first-column-description", " https://adblockplus.org/terms");

[Thomas Greiner, 2018/02/16 16:19:13]

As done with the Updates page, don't hardcode any links but instead use
documentation links.

     setLinks("third-column-description", "https://adblockplus.org/acceptable-ads#optout");
     setLinks("copyright-notice", "https://eyeo.com");
-  }
 
-  function openFilters()
-  {
-    browser.runtime.sendMessage({type: "app.open", what: "options"});
+    initMenu();
   }
 
   document.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 }());