Problem
This is a followup post to version 1, and version 2.
The following code is a website with a nav-bar on it. It uses some simple JavaScript and CSS (with media-queries) to make it work:
index.html:
<!DOCTYPE html>
<html lang="en-US">
<head>
<meta charset="UTF-8" />
<title>SigmaCubes</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>
<div>
<noscript>
<div class="error">
<h1>Error:</h1>
<p class="error-type">JavaScript</p>
</div>
</noscript>
<!--[if IE]>
<div class="error">
<h1>Error:</h1>
<p class="error-type">Old Internet Explorer</p>
</div>
<![endif]-->
<div id="nav-stupid" class="error">
<h1>Error:</h1>
<p class="error-type">Resolution</p>
</div>
</div>
<div class="hidden" id="wrapper">
<div id="nav-large">
<img alt="SigmaCubes Logo" class="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 id="nav-small">
<img alt="SigmaCubes Logo" class="logo_h" height="24" src="/img/logo_h.svg" />
<a class="nav-toggle" onclick="toggleNav()" href="javascript:void(0);">
<img height="28" src="/img/menu.svg" />
</a>
</div>
<div class="hidden" id="dropdown">
<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 id="content">test</div>
</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
html, body {
background-color: #ddd;
font-family: "Noto Sans", sans-serif;
height: 100%;
}
h1 {
font-size: 24px;
margin: 4px 16px;
}
@media screen and (max-width: 250px) {
#nav-stupid {
display: block;
}
#content, #dropdown, #nav-large, #nav-small {
display: none;
}
}
@media screen and (min-width: 251px) and (max-width: 500px) {
#nav-small {
display: block;
}
#nav-large, #nav-stupid {
display: none;
}
}
@media screen and (min-width: 501px) {
#nav-large {
display: block;
}
#dropdown, #nav-small, #nav-stupid {
display: none;
}
}
.block {
display: block;
}
.error {
padding: 8px;
}
.error-type {
font-weight: bold;
margin: 4px 16px;
}
.hidden {
display: none;
}
.link-large {
color: #000;
padding: 0 8px;
text-decoration: none;
}
.link-large:hover {
color: #00f;
}
.logo_h {
left: 16px;
position: absolute;
top: 10px;
}
.link-small {
color: #fff;
display: block;
padding: 8px;
text-decoration: none;
}
.link-small:hover {
color: #88f;
}
.nav-toggle {
position: absolute;
right: 16px;
top: 10px;
}
#content {
background-image: url("/img/hq.jpg");
color: #fff;
padding: 16px;
}
#dropdown {
background-color: #393f47;
text-align: left;
}
#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;
}
#wrapper {
text-align: center;
}
main.js
var navEnabled = false;
document.addEventListener('DOMContentLoaded', main);
function main() {
setDisplay("wrapper", "block");
}
function toggleNav() {
if (navEnabled) {
setDisplay("dropdown", "hidden");
} else {
setDisplay("dropdown", "block");
}
navEnabled = !navEnabled;
}
function setDisplay(id, className) {
document.getElementById(id).className = className;
}
Solution
In the HTML, the logo image appears to be duplicated – once under the nav-large div and also under the nav-small element. However, there don’t appear to be any specific styles on the logo image relative to the parent container, so that could be moved up above those without any side-effect. That way, if the logo URL needed to be updated, it could be changed in one spot instead of two.
Additionally, (just as sumurai8 mentioned a couple months ago: ) “you are duplicating your menu“1 The HTML could be greatly simplified by utilizing the media queries more. For example, consider, moving the toggle image out of the navigation menu, and eliminating the specific link classes (i.e. link-large and link-small):
<div class="hidden" id="wrapper">
<img alt="SigmaCubes Logo" class="logo_h" height="24" src="http://sigmacubes.com/img/logo_h.svg" />
<a class="nav-toggle" href="javascript:void(0);">
<img height="28" src="http://sigmacubes.com/img/menu.svg" />
</a>
<div id="nav-menu">
<div id="link-container">
<a class="link" href="#">About Us</a>
<a class="link" href="#">Projects</a>
<a class="link" href="#">Contact</a>
<a class="link" href="#">Donate</a>
</div>
</div>
<div id="content">test</div>
</div>
Then in the CSS, style those elements appropriately within the media query blocks:
@media screen and (min-width: 251px) and (max-width: 500px) {
.link {
color: #fff;
display: block;
padding: 8px;
text-decoration: none;
}
.link:hover {
color: #88f;
}
@media screen and (min-width: 501px) {
.link-large {
color: #000;
padding: 0 8px;
text-decoration: none;
}
.link-large:hover {
color: #00f;
}
There are a few other elements and styles that need to be changed to get that to work. See the rewritten code in the snippet below.
While it may not be much of an issue for a small page like this one, it is wise to cache DOM references, instead of looking them up each time an element is altered. You could change the setDisplay() function to accept a DOM element:
function setDisplay(domElement, className) {
domElement.className = className;
}
And then pass elements to it. For instance, toggleNav would pass a reference to the element with id attribute dropdown. That could be declared at the top near navEnabled (or anywhere within the js really, since var is used; if let was used, then it would need to be at the top) and assigned in main or another function that is run when the DOM is ready (though with modern browsers it could likely be handled in the JS if the script is included within the <body> tag).
Additionally, the name of that function could be a bit more reflective of what it does. Sure it sets the display by setting the class name but it really just sets the class name of the element. So a more appropriate name would be something like setClassNameOnElement.
Sumurai8’s answer suggested using an IIFE to avoid variable name conflicts. It would be wise to heed that advice.
Sumurai8’s answer answer also mentioned:
Similary you can attach click handlers to DOM elements itself:
document.getElementById('my-element').addEventListener('click', myClickHandler);
So instead of having the onclick attribute for the anchor (link) element with class name nav-toggle, the Javascript code could add the click listener for that. With this approach, the layout (HTML) and logic (Javascript) are separated so in theory multiple team members could work on the code and one wouldn’t have to update the HTML code just to update the logic.
For this, you could add an id attribute to that anchor (link) tag, or else utilize document.querySelector('.nav-toggle').
If there were multiple elements that need to have click events handled, it might be wise to consider using an event delegate on the body element or an child element that contains all elements to observe click events on
According to MSDN Docs:
Support for conditional comments has been removed in Internet Explorer 10 standards and quirks modes for improved interoperability and compliance with HTML5. This means that Conditional Comments are now treated as regular comments, just like in other browsers. 2
So those condition comments won’t have any effect on IE 10 or 11. If you want to display the IE message, you may need to utilize JavaScript (see How to Detect Features Instead of Browsers for more information)
For updated code, see the snippet below: