3
\$\begingroup\$

The following code is a website with a nav-bar on it. It uses some simple JavaScript to make it work:

index.html:

<!DOCTYPE html>
<html lang="en-US">
<head>
  <meta charset="UTF-8" />
  <title>SigmaCubes - Home</title>
  <meta name="author" content="Julian Lachniet,Simon Kwilinski,Jacob Wysko" />
  <meta name="description" content="SigmaCubes is the best website on the internet." />
  <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0" />
  <link href="https://fonts.googleapis.com/css?family=Lato|Noto+Sans|Nunito:600|Material+Icons" rel="stylesheet" type="text/css" />
  <link href="/lib/css/reset.css" rel="stylesheet" type="text/css" />
  <link href="/lib/css/sigmacubes.css" rel="stylesheet" type="text/css" />
  <link href="/favicon.ico" rel="shortcut icon" />
  <script src="/lib/js/main.js"></script>
</head>
<body onload="main()">

  <noscript>
    <div class="error">
      <h1>Error:</h1>
      <h2>Your browser does not currently support JavaScript which is required to view this page.</h2>
    </div>
  </noscript>

  <!--[if IE]>
    <div class="error">
      <h1>Error:</h1>
      <h2>Your browser is not supported by SigmaCubes.</h2>
    </div>
  <![endif]-->

  <div class="hidden" id="nav-large">
    <img alt="SigmaCubes Logo" id="logo_h" height="24" src="/img/logo_h.svg" />
    <div id="nav-large-links">
      <a class="link-large" href="#">About Us</a>
      <a class="link-large" href="#">Projects</a>
      <a class="link-large" href="#">Contact</a>
      <a class="link-large" href="#">Donate</a>
    </div>
  </div>

  <div class="hidden" id="nav-small">
    <img alt="SigmaCubes Logo" id="logo_h" height="24" src="/img/logo_h.svg" />
    <a class="link-nav" onclick="toggleNav()" href="javascript:void(0);">
      <img height="28" src="/img/menu.svg" />
    </a>
  </div>

  <div class="hidden" id="nav-small-links">
    <a class="link-small" href="#">About Us</a>
    <a class="link-small" href="#">Projects</a>
    <a class="link-small" href="#">Contact</a>
    <a class="link-small" href="#">Donate</a>
  </div>

  <div class="hidden" id="nav-stupid">
    <div class="error">
      <h1>Error:</h1>
      <h2>Your window size is too small to view this page correctly. Please resize the window.</h2>
    </div>
  </div>

  <div id="wrapper">
    <img id="hq" src="img/hq.jpg" />
  </div>

</body>
</html>

reset.css:

html, body, div, span, applet, object, iframe, h1, h2, h3, h4, h5, h6, p,
blockquote, pre, a, abbr, acronym, address, big, cite, code, del, dfn, em, img,
ins, kbd, q, s, samp, small, strike, strong, sub, sup, tt, var, b, u, i,
center, dl, dt, dd, ol, ul, li, fieldset, form, label, legend, table, caption,
tbody, tfoot, thead, tr, th, td, article, aside, canvas, details, embed,
figure, figcaption, footer, header, hgroup, menu, nav, output, ruby, section,
summary, time, mark, audio, video {
  margin: 0;
  padding: 0;
  border: 0;
  font-size: 100%;
  font: inherit;
  vertical-align: baseline;
}

article, aside, details, figcaption, figure, footer, header, hgroup, menu, nav,
section {
  display: block;
}

body {
  line-height: 1;
}

ol, ul {
  list-style: none;
}

blockquote, q {
  quotes: none;
}

blockquote:before, blockquote:after, q:before, q:after {
  content: "";
  content: none;
}

table {
  border-collapse: collapse;
  border-spacing: 0;
}

sigmacubes.css:

body {
  background-color: #ddd;
  font-family: "Noto Sans", sans-serif;
}

h1 {
  font-size: 24px;
  margin: 4px 16px;
}

h2 {
  font-size: 18px;
  margin: 16px;
}

.error {
  padding: 16px;
}

.hidden {
  display: none;
}

.link-large {
  color: #000;
  padding: 0 8px;
  text-decoration: none;
}

.link-large:hover {
  color: #00f;
}

.link-nav {
  position: absolute;
  right: 16px;
  top: 10px;
}

.link-small {
  color: #fff;
  display: block;
  padding: 8px;
  text-decoration: none;
}

.link-small:hover {
  color: #88f;
}

#hq {
  width: 100%;
}

#logo_h {
  left: 16px;
  position: absolute;
  top: 10px;
}

#wrapper {
  text-align: center;
}

#nav-large, #nav-small {
  background-color: #63a4ff;
  font-family: "Nunito", sans-serif;
  font-size: 16px;
  height: 16px;
  padding: 16px 0;
  text-align: right;
}

#nav-large-links {
  display: inline;
  margin: 0 16px;
}

#nav-small-links {
  background-color: #393f47;
}

main.js:

var screenMode = "none", nav = false;

function main() {
  setInterval(function(){updateLayout()}, 10);
}

function updateLayout() {
  var width = window.innerWidth;
  if (width < 250 & screenMode != "stupid") {
    screenMode = "stupid"
    setDisplay("nav-stupid", "block");
    setDisplay("nav-small", "none");
    setDisplay("nav-small-links", "none");
    setDisplay("nav-large", "none");
  } else if (width > 249 && width < 500 && screenMode != "mobile") {
    screenMode = "mobile"
    setDisplay("nav-stupid", "none");
    setDisplay("nav-small", "block");
    setDisplay("nav-small-links", "none");
    setDisplay("nav-large", "none");
  } else if (width > 499 && screenMode != "desktop") {
    screenMode = "desktop"
    setDisplay("nav-stupid", "none");
    setDisplay("nav-small", "none");
    setDisplay("nav-small-links", "none");
    setDisplay("nav-large", "block");
  }
}

function toggleNav() {
  if (nav) {
    setDisplay("nav-small-links", "none");
    nav = false;
  } else {
    setDisplay("nav-small-links", "block");
    nav = true;
  } 
}

function setDisplay(id, state) {
  document.getElementById(id).style.display = state;
}

Temporary live demo

\$\endgroup\$
3
  • 2
    \$\begingroup\$ The live demo doesn't appear to do anything. \$\endgroup\$
    – Mast ♦
    Commented Apr 29, 2018 at 9:48
  • \$\begingroup\$ It did work for me @Mast \$\endgroup\$
    – Sumurai8
    Commented Apr 29, 2018 at 11:04
  • \$\begingroup\$ Can you insert the code into a stack snippet? Press "Ctrl+M" in the body of the post to bring that editor up. \$\endgroup\$
    – Dan Oberlam
    Commented Apr 30, 2018 at 13:48

1 Answer 1

Reset to default
7
\$\begingroup\$

General disclaimer: You may notice that in one section I recommend not using something in this case, while in the next paragraph I show how you can improve the part I recommended not to use. I mainly do this so you learn from it for other parts of your website or future projects.

Media queries > javascript

The main problem I have with your code is that you are duplicating your menu and are using javascript to switch between a mobile and desktop view. CSS has media queries that allow you to completely alter how something is rendered based on the client width and height.

Media queries are blocks around css in the form of:

@media (screen and (max-width: 420px)) {
  /* css here */
}

The wrapped css will only be applied when the conditions are met. In this case, the css will only be applied when shown on a screen (as opposed to a beamer or a printed version) and the window width does not exceed 420 pixels. By using css you are making your code less complex and faster to render. By de-duplicating your html you make your code easier to maintain in the long run.

Avoid onload and other such attributes

Try to avoid onload, onclick and other javascript event attributes. Instead, attach handlers in your javascript file itself. The easiest way to do this is by using the DOMContentLoaded event which is supported in all modern browsers.

document.addEventListener('DOMContentLoaded', main);

Similary you can attach click handlers to DOM elements itself:

document.getElementById('my-element').addEventListener('click', myClickHandler);

nav-stupid

I would suggest removing nav-stupid. While it is true that it is kinda stupid to have a screen smaller than the smallest mobile device, this does not serve any purpose for anyone. Chances are that someone that has their device width that small knows that pretty much any site will break in some way. At least leave them with a (partial broken) mobile menu.

Avoid busy loops

You are currently using an interval to detect if the window width has changed. Intervals should be your last resort. If there is a DOM event you can listen to, this is a much better alternative. Intervals will always be executed where DOM events are only fired when things actually change. The intervals will thus consume cpu cycles even if nothing has to happen.

In your case you could listen to the resize event. You will have one problem though, and that is that the callback function will be called several hundreds of times in a short amount of time. You can get around this problem by using requestAnimationFrame() or by using a throttle function.

// Basic throttle function
var throttledResizeTimeoutRef = null;
function throttledResize() {
  var timeout = 20; // ms

  if (throttledResizeTimeoutRef) {
    window.clearTimeout(throttledResizeTimeoutRef);
  }

  throttledResizeTimeoutRef = window.setTimeout(
    function () {
      throttledResizeTimeoutRef = null;
      resize();
    },
    timeout
  );
}

function resize() {
  // Code that should be executed on resize
}

Use function references

When you declare a function using function ticklePolarBear() { ... }, then ticklePolarBear is a function reference. Putting () behind that reference executes that function.

Why is this important? Well, you are using window.setInterval(..). Notice that you are declaring an anonymous function inside the parenthesis of that method call. This creates a function reference that can be used by the code of setInterval(..). Instead of doing that, you could simplify setInterval(function(){updateLayout()}, 10); to window.setInterval(updateLayout, 10);.

Namespacing

By default, when you declare a function or variable in javascript, you declare it in the global namespace (aka as attribute of window). While you sometimes can't get around this, it is generally better to wrap all parts in their own contained namespace. This avoids problems where two separate pieces of code declare the same variable or function and end up messing with each other and makes debugging easier as you do not have to wade through a mess of variables under the window object.

You can 'namespace' your code by wrapping the code in a so-called IIFE. 

(function () {
  // Your code here
})();

If your code has any dependencies, such as a library as jQuery, you pass them as arguments.

(function ($) {
  // Your code here
  // You can use jQuery as $
})(jQuery);

(function ($) {
  // Some other code
  // You can use prototype as $
})(prototype);

Inline styling

You are modifying the page by altering inline styling of your elements. Consider modifying the classes on these elements instead. There are several benefits for doing that:

  • Styling needs to be defined once
  • If you need to override styling, it is a hell to override styling applied as inline styling. In your case that may not be a problem, but if you have ever worked on a website where you needed to change the appearance of, for example, a slider that applies styling inline you will understand the pain of trying to get your styling to stick.

Minor issues and typos

In general your code is clean and reasonably easy to read. You have declared all your variables so that they do not end up in the global scope (if you used the advice above). Your functions do what I expect them to do based on their names. Your css is reasonably sparse (as in: you are not overly complicating the css by overwriting styling for no apparent reason). You are also using alt texts for your images, which is a good thing for screenreaders. Those are all good things ;-)

In if (width < 250 & screenMode != "stupid") { you are using a bitwise AND operator instead of the logical AND operator (&&).

I recommend strict comparison over the non type-strict comparisons you are currently doing. For example 1 == true will resolve to true, while 1 === true will resolve to false. By using the triple === and !== you protect yourself against having to debug weird behaviour caused by type conversions where you don't want them to happen. If you need type conversions, you can always explicitly convert the types.

I recommend not using the comma notation when declaring variables. Instead just declare your variables one at a time and one per line. It protects you against accidentally dropping variables in the global scope and makes git diffs easier to read if you are using version control.

Because you are duplicating your html, you have duplicated your logo element. Elements with an id should be unique within a document. In your case, #logo_h appears more than once on the page. This may cause unintended behaviour and I would recommend fixing this.

You could add a doctype to the top of your html document. Just add the following as the first line:

<!DOCTYPE html>

You are using width > 249 and width > 499. Consider using width >= 250 and width >= 500 so the numbers match the clause above. This is particularly useful if you are using with floating point numbers rather than integers.

Blasphemy

Your description meta-tag says "SigmaCubes is the best website on the internet.". I thought Code Review already held that place ;-)

On a more serious note: I would recommend changing that to an actual description for SEO reasons.

\$\endgroup\$
1
  • \$\begingroup\$ "By functions do what I expect them to do based on their names" should By have been a different word, like The? \$\endgroup\$
    – Sᴀᴍ Onᴇᴌᴀ ♦
    Commented Jun 21, 2018 at 16:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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