Issue 5482 - Sidebar and about ABP dialog

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -550,17 +550,6 @@
     });
   }
 
-  function openDocLink(id)
-  {
-    getDocLink(id, (link) =>
-    {
-      if (id == "share-general")
-        openSharePopup(link);
-      else
-        location.href = link;
-    });
-  }
-
   function switchTab(id)
   {
     location.hash = id;
@@ -652,11 +641,6 @@
         openDialog(dialog);
         break;
       }
-      case "open-doclink": {
-        let doclink = findParentData(element, "doclink", false);
-        openDocLink(doclink);
-        break;
-      }
       case "remove-filter":
         ext.backgroundPage.sendMessage({
           type: "filters.remove",
@@ -811,6 +795,13 @@
     }
   }
 
+  function updateTabLinks()

[juliandoucette, 2017/08/21 14:10:34]

NIT: Why not add these HREFs directly to new-options.html?

[saroyanm, 2017/08/21 15:20:22]

To avoid duplications, we do specify the target in datasets, so this should be
specified in 1 place, please note we don't optimizing here for SEO.

[juliandoucette, 2017/08/21 16:07:24]

NIT: You could *just* set a.href then and:

tabList.querySelector("li[aria-controls='content-" + tabId + "']")

OR

tabList.querySelector("li#tab-" + tabId + "']")

OR

tabList.querySelector("a[href='#" + tabId + "']").parentElement

elsewhere then. But I don't insist. 

[saroyanm, 2017/08/21 16:46:12]

I agree with you, I'll change this.

[saroyanm, 2017/08/23 13:35:45]

I've updated the tabs implementation.

+  {
+    let tabs = document.querySelectorAll("[role='tab']");

[juliandoucette, 2017/08/21 14:10:34]

NIT: Isn't this asking for trouble? e.g. if we add non-sidebar tabs in the
future

Suggest: "#sidebar .tabs a" (or equivalent)

[saroyanm, 2017/08/21 15:20:22]

Why is this trouble ? No, this logic I think should be general, but not sidebar
specific, until you intentionally making it sidebar specific, I don't think that
it's a case, here.

[juliandoucette, 2017/08/21 16:07:24]

Acknowledged.

Assuming that you want this to happen to all (not just sidebar) tabs in this
document in the future.

+    for (let i = 0; i < tabs.length; i++)
+      tabs[i].querySelector("a").href = "#" + tabs[i].dataset.tab;
+  }
+
   function selectTabItem(tabId, container, focus)
   {
     // Show tab content
@@ -867,15 +858,12 @@
     },
     (addonVersion) =>
     {
-      E("abp-version").textContent = addonVersion;
-    });
-    getDocLink("releases", (link) =>
-    {
-      E("link-version").setAttribute("href", link);
+      E("abp-version").textContent = getMessage("options_dialog_about_version",
+        [addonVersion]);
     });
 
-    updateShareLink();
     updateTooltips();
+    updateTabLinks();
 
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, false);
@@ -888,6 +876,11 @@
       E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
+
+    getDocLink("contribute", (link) =>

[juliandoucette, 2017/08/21 14:10:34]

NIT: It seems like these should be batched somewhere e.g.

setDocLinks([
  [DOC_ID, DOM_ID, ATTR_NAME],
  [DOC_ID, DOM_ID, ATTR_NAME]
]);

Perhaps automatically via HTML attributes or className style.

[saroyanm, 2017/08/21 15:20:22]

Yes it's a plan for future. We might also have a ticket for that. If not we can
create one. Again, future enhancement or at least separate from this review.

[juliandoucette, 2017/08/21 16:07:25]

Acknowledged.

Will you check this?

[saroyanm, 2017/08/21 16:46:13]

https://issues.adblockplus.org/ticket/4856

[juliandoucette, 2017/08/22 10:10:44]

Acknowledged.

+    {
+      E("contribute").href = link;
+    });
     getDocLink("acceptable_ads_criteria", (link) =>
     {
       setLinks("enable-aa-description", link);
@@ -1163,7 +1156,6 @@
       case "added":
         filter[timestampUI] = Date.now();
         updateFilter(filter);
-        updateShareLink();
         break;
       case "loaded":
         populateLists();
@@ -1176,7 +1168,6 @@
           removeCustomFilter(filter.text);
 
         delete filtersMap[filter.text];
-        updateShareLink();
         break;
     }
   }
@@ -1244,7 +1235,6 @@
         break;
     }
 
-    updateShareLink();
   }
 
   function hidePref(key, value)
@@ -1305,30 +1295,6 @@
       checkbox.setAttribute("aria-checked", value);
   }
 
-  function updateShareLink()
-  {
-    let shareResources = [
-      "https://facebook.com/plugins/like.php?",
-      "https://platform.twitter.com/widgets/",
-      "https://apis.google.com/se/0/_/+1/fastbutton?"
-    ];
-    let isAnyBlocked = false;
-    let checksRemaining = shareResources.length;
-
-    function onResult(isBlocked)
-    {
-      isAnyBlocked |= isBlocked;
-      if (!--checksRemaining)
-      {
-        // Hide the share tab if a script on the share page would be blocked
-        E("tab-share").hidden = isAnyBlocked;
-      }
-    }
-
-    for (let sharedResource of shareResources)
-      checkShareResource(sharedResource, onResult);
-  }
-
   function getMessages(id)
   {
     let messages = [];