13
\$\begingroup\$

I've been annoyed with the new side bar layout, so I chose to hide it. But then, the useful links were gone, so I decided to make this little user script in Tampermonkey to bring them back to [roughly] how it looked before. Here is a screenshot:

screenshot

It's nothing very complicated, and it's all plain regular JavaScript, but I'd like any suggestions to improve it before I put it on Stack Apps for everyone to see!

Note that design suggestions are also welcome, but I'd prefer if those were made in comments or in chat.

// ==UserScript==
// @name         Stack Overflow custom top navbar
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  Make Stack Overflow Design Great Again #MSODGA :)
// @author       https://codereview.stackexchange.com/users/42632/phrancis
// @include      http*://*.stackoverflow.com*
// @include      http*://stackoverflow.com*
// @grant        none
// ==/UserScript==

(function() {
    "use strict";

    const stackOverflowOrange = "#F48024";

    // User of script can set these preferences to modify the script's behavior
    const userPreferences = {
        // Change to true to hide the "Jobs" Stack Overflow Careers link
        hideJobsLink : false,
        // Set your own colors for the linkss here
        // Make sure to use valid CSS colors like "#f48024" or "black"
        linkColor : "black",
        linkHoverColor : stackOverflowOrange,
        linkClickColor : stackOverflowOrange
    };

    // It is recommended to not change anything below this comment
    // unless you know JavaScript, as it can break the functionality.

    const links = [
        { name: "Home", url: "https://stackoverflow.com/" },
        { name: "Questions", url: "https://stackoverflow.com/questions" },
        { name: "Tags", url: "https://stackoverflow.com/tags" },
        { name: "Users", url: "https://stackoverflow.com/users" }
    ];

    if (!userPreferences.hideJobsLink) {
        links.push({ name: "Jobs", url: "https://stackoverflow.com/jobs?med=site-ui&ref=jobs-tab" });
    };

    const navbarContainer = document.createElement("div");
    navbarContainer.id = "navbar-container";
    document.getElementById("content").prepend(navbarContainer);

    const navbarList = document.createElement("ul");
    navbarList.id = "navbar-list";
    navbarContainer.append(navbarList);

    for (let item of links) {
        const listItem = document.createElement("li");
        listItem.className = "navbar-list-item";
        const link = document.createElement("a");
        link.className = "navbar-link";
        link.href = item.url;
        link.innerHTML = item.name;
        listItem.append(link);
        navbarList.append(listItem);
    }

    /**
     * Add CSS properties to an HTMLCollection queried by document.querySelectorAll
     * @param {string} selectorAllQuery
     *     a valid query to be used by document.querySelectorAll
     * @param {object} cssObject
     *     an object containing pairs of CSS property names (in JS format, like listStyleTyle rather
     *     than 'list-style-type' and a value to assign to it
     * @return {undefined}
     */
    const applyCssToHTMLCollection = function(selectorAllQuery, cssObject) {
        let elements = null;
        try {
            elements = document.querySelectorAll(selectorAllQuery);
        } catch(exception) {
            throw `Invalid document.querySelectorAll query expression: ${selectorAllQuery}`;
        }
        for (let element of elements) {
            Object.assign(element.style, cssObject);
        }
    };

    // Style inspired by example at the following link:
    // https://www.w3schools.com/css/tryit.asp?filename=trycss_float5

    applyCssToHTMLCollection("#navbar-list", {
        listStyleType: "none",
        margin: 0,
        padding: 0,
        overflow: "hidden",
        textAlign: "center"
    });
    applyCssToHTMLCollection("li.navbar-list-item", {
        float: "left"
    });
    applyCssToHTMLCollection("li.navbar-list-item > a", {
        display: "inline-block",
        color: userPreferences.linkColor,
        textAlign: "center",
        padding: "14px 16px",
        textDecoration: "none",
        fontSize: "1.4em",
        fontWeight: "bold"
    });
    const navbarLinks = document.querySelectorAll(".navbar-link");
    for (let element of navbarLinks) {
        element.addEventListener("mouseenter", function() {
            element.style.color = userPreferences.linkHoverColor;
        });
        element.addEventListener("mouseleave", function() {
            element.style.color = userPreferences.linkColor;
        });
        element.addEventListener("click", function() {
            element.style.color = userPreferences.linkClickColor;
        });
    }

})();
\$\endgroup\$
6
\$\begingroup\$

That's generally a really neat code you've got there. Here are my remarks, though:


Applying styles and position of it in code

Instead of applyCssToHTMLCollection() and especially 3 event listeners, you could create <style> element, set its .textContent once and append it to body.

Also, to prevent flash of unstyled content styles should be already in place before the elements are added to the body. For the same reason I would move document.getElementById("content").prepend(navbarContainer) beyond the following for … of.


throw {…} catch(…) { in applyCssToHTMLCollection()

In applyCssToHTMLCollection() you throw if querySelectorAll() throws, which will happen only if it's passed invalid CSS selector. Only selectors that can get to it are:

  • #navbar-list,
  • li.navbar-list-item and
  • li.navbar-list-item > a

Since they are all valid, you will never reach that catch() and it is therefore needless to use it.

Also, it would be good to add @throws to this function's JSDoc.


let vs cons in for … of

In three for … ofs you use let instead of const. Despite intuitively it seems that, after all, that variable is going to change with the next iteration, we can actually use const as long as we are not going to assign anything to it inside that loop.


Format

Compare readability of this:

for (let item of links) {
    const listItem = document.createElement("li");
    listItem.className = "navbar-list-item";
    const link = document.createElement("a");
    link.className = "navbar-link";
    link.href = item.url;
    link.innerHTML = item.name;
    listItem.append(link);
    navbarList.append(listItem);
}

to this:

for (let item of links) {
  const listItem     = document.createElement("li");
  listItem.className = "navbar-list-item";

  const link     = document.createElement("a");
  link.className = "navbar-link";
  link.href      = item.url;
  link.innerHTML = item.name;

  listItem.append(link);
  navbarList.append(listItem);
}

Style

JavaScript Standard Style specifies use of two spaces for indentation and use of single quotes for strings. It's also more popular option too. While style is matter of personal taste and habit of course, I think it's still worth considering.

Rewrite

// ==UserScript==
// @name         Stack Overflow custom top navbar
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  Make Stack Overflow Design Great Again #MSODGA :)
// @author       https://codereview.stackexchange.com/users/42632/phrancis
// @include      http*://*.stackoverflow.com*
// @include      http*://stackoverflow.com*
// @grant        none
// ==/UserScript==

(function() {
  'use strict';

  const stackOverflowOrange = '#F48024';

  // User of script can set these preferences to modify the script's behavior
  const userPreferences = {
    // Change to true to hide the "Jobs" Stack Overflow Careers link
    hideJobsLink: false,

    // Set your own colors for the links here.
    // Make sure to use valid CSS colors like "#f48024" or "black"
    linkColor:      'black',
    linkHoverColor: stackOverflowOrange,
    linkClickColor: stackOverflowOrange
  };

  // It is recommended to not change anything below this comment
  // unless you know JavaScript, as it can break the functionality
  const links = [
    { name: 'Home',      url: 'https://stackoverflow.com/' },
    { name: 'Questions', url: 'https://stackoverflow.com/questions' },
    { name: 'Tags',      url: 'https://stackoverflow.com/tags' },
    { name: 'Users',     url: 'https://stackoverflow.com/users' }
  ];

  if (!userPreferences.hideJobsLink) {
    links.push({ name: 'Jobs', url: 'https://stackoverflow.com/jobs?med=site-ui&ref=jobs-tab' });
  }

  // Set styles
  const style = document.createElement('style');
  style.textContent =
    `#navbar-list {
      list-style-type: none;
      margin: 0;
      overflow: hidden;
      padding: 0;
      text-align: center;
    }
    li.navbar-list-item {
      float: left;
    }
    li.navbar-list-item > a {
      color: ${userPreferences.linkColor};
      display: inline-block;
      font-size: 1.4em;
      font-weight: bold;
      padding: 14px 16px;
      text-align: center;
      text-decoration: none;
    }
    .navbar-link {
      color: ${userPreferences.linkColor};
    }
    .navbar-link:hover {
      color: ${userPreferences.linkHoverColor};
    }
    .navbar-link:focus {
      color: ${userPreferences.linkClickColor};
    }`;
  document.body.appendChild(style);

  // Create links
  const navbarContainer = document.createElement('div');
  navbarContainer.id    = 'navbar-container';

  const navbarList = document.createElement('ul');
  navbarList.id    = 'navbar-list';
  navbarContainer.append(navbarList);

  for (const item of links) {
    const listItem     = document.createElement('li');
    listItem.className = 'navbar-list-item';

    const link = document.createElement('a');
    [link.className, link.href, link.textContent] = ['navbar-link', item.url, item.name];

    listItem.append(link);
    navbarList.append(listItem);
  }

  document.getElementById('content').prepend(navbarContainer);
})();
\$\endgroup\$
  • 1
    \$\begingroup\$ JavaScript standard style also recommends no semicolons and no extraneous spacing between operators (e.g. like you use to align your assignment operators). I only point that out because it's not clear why you follow some guidelines while ignoring others. By the way, your line assigning to link near the bottom should probably be changed to Object.assign(link, { className: 'navbar-link', href: item.url, textContent: item.name }) for readability. \$\endgroup\$ – Patrick Roberts Jun 16 '18 at 15:44
  • \$\begingroup\$ @PatrickRoberts: Yeah, I know about those semicolons and stuff, but as I've said, it's mostly about personal taste. I just wanted to note presence of trend to use single quotes and 2 spaces for indentation. Regarding readability of the latter, I find that destructuring assignment as clear. \$\endgroup\$ – Przemek Jun 16 '18 at 15:53
  • 1
    \$\begingroup\$ Just keep in mind if you ever decide to remove semicolons from your script, that line will either need to be changed as I suggested, or changed to begin with a semicolon to prevent the script from throwing ReferenceError: link is not defined, since the destructuring would actually be interpreted as a property accessor of document.createElement('li') \$\endgroup\$ – Patrick Roberts Jun 16 '18 at 16:00
  • 1
    \$\begingroup\$ @PatrickRoberts: And that's why I like semicolons ;) \$\endgroup\$ – Przemek Jun 16 '18 at 16:11
  • 4
    \$\begingroup\$ I think there's some confusion about the "JavaScript standard". This is something (plugin/extension/package?) made by a person Feross Aboukhadijeh. MDN consistently use double quotes and semicolons throughout their documentation (they do use 2 spaces too). I do prefer 4 spaces for readability, and either way it doesn't affect the code, as long as it's consistent. \$\endgroup\$ – Phrancis Jun 16 '18 at 17:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.