7
\$\begingroup\$

I'm fairly new to HTML and CSS and I've put together a navigation bar. The purpose of the navigation bar is to be clean and mobile-friendly. I would appreciate some feedback on what I could do better within the code as I don't know what's best just yet.

const navSlide = () => {
  const burger = document.querySelector('.burger');
  const nav = document.querySelector('.nav-links');
  const navLinks = document.querySelectorAll('.nav-links li');
  //togle nav
  burger.addEventListener('click', () => {
    nav.classList.toggle('nav-active');

    navLinks.forEach((link, index) => {
      if (link.style.animation) {
        link.style.animation = '';
      } else {
        link.style.animation = `navLinkFade 0.5s ease forwards ${index / 7 + .3}s`;
      }
    });

    burger.classList.toggle('toggle');
  });
}

navSlide();
@import url('https://fonts.googleapis.com/css?family=Montserrat:400,500,700&display=swap');
@import url('https://fonts.googleapis.com/css?family=Fira+Sans:400,400i,500,500i,700,700i&display=swap');
* {
  margin: 0;
  padding: 0;
}

body {
  background-color: #E5ECE9;
  height: 2000px;
  font-family: 'montserrat', sans-serif;
}

nav {
  display: flex;
  justify-content: space-between;
  align-items: center;
  min-height: 8vh;
  background-color: white;
  flex-basis: 1336px;
  padding-left: 10%;
  padding-right: 10%;
}

.logo img {
  height: 84px;
  max-width: 100%;
}

.nav-links {
  display: flex;
  justify-content: space-around;
}

.nav-links li {
  list-style-type: none;
  padding: 10px 10px 10px;
}

.nav-links a {
  color: black;
  text-decoration: none;
  letter-spacing: 2px;
  font-weight: 500;
  font-family: 'montserrat', sans-serif;
  font-size: 14px;
}

.nav-links #green-select {
  font-weight: bold;
  color: #A7E66E;
}

.burger {
  display: none;
  cursor: pointer;
}

.burger div {
  width: 25px;
  height: 3px;
  background-color: black;
  margin: 3px;
  transition: all 0.3s ease;
}

.landing-wrapper {
  width: 100%;
  background-color: white;
  height: 694px;
}

.landing {
  width: 1336px;
  position: absolute;
  top: 40%;
  left: 50%;
  transform: translate(-50%, -50%);
}

.landing h1 {
  text-align: center;
  font-size: 70px;
  font-family: 'Fira Sans', sans-serif;
  font-style: italic;
  line-height: 90%;
  padding-bottom: 20px;
}

.landing p {
  text-align: center;
  font-size: 18px;
  font-family: 'montserrat', sans-serif;
  padding-left: 15%;
  padding-right: 15%;
  line-height: 1.6;
}


/* Responsive Design */

@media screen and (max-width: 1024px) {}

@media screen and (max-width:768px) {
  body {
    overflow-x: hidden;
  }
  .nav-links {
    position: absolute;
    right: 0px;
    height: 92vh;
    top: 9vh;
    background-color: white;
    display: flex;
    flex-direction: column;
    align-items: center;
    width: 100%;
    transform: translateX(100%);
    transition: transform 0.5s ease-in;
  }
  .nav-links li {
    opacity: 0;
  }
  .burger {
    display: block;
  }
}

.nav-active {
  transform: translateX(0%);
}

@keyframes navLinkFade {
  from {
    opacity: 0;
    transform: translateX(50px);
  }
  to {
    opacity: 1;
    transform: translateX(0px);
  }
}

.toggle .line1 {
  transform: rotate(-45deg) translate(-5px, 6px);
}

.toggle .line2 {
  opacity: 0;
}

.toggle .line3 {
  transform: rotate(45deg) translate(-5px, -6px);
}
<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <link rel="stylesheet" type="text/css" href="style.css">
  <meta http-equiv="Cache-control" content="no-cache">
  <meta http-equiv="X-UA-Compatible" content="ie=edge">
  <title>Navigation Tutorial</title>
</head>

<body>
  <nav>
    <div class="logo">
      <img src="logo.png">
    </div>
    <ul class="nav-links">
      <li><a id="green-select" href="#">Showcase</a></li>
      <li><a href="#">About</a></li>
      <li><a href="#">Blog</a></li>
      <li><a href="#">Contact</a></li>
    </ul>
    <div class="burger">
      <div class="line1"></div>
      <div class="line2"></div>
      <div class="line3"></div>
    </div>
  </nav>
  <script src="app.js"></script>
</body>

</html>

\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Overall Feedback

For a beginner this code looks great!. It appears to function correctly as per design. I like that the Javascript code uses small arrow functions, as well as the const keyword instead of let as variables are never re-assigned, which helps avoid accidental re-assignment. Indentation is very consistent.

Suggestions

Presuming there will be only one burger element and one navigation list use the id attribute instead of a classname for the elements with class names burger and nav-links. Obviously this affects the CSS selectors. In the Javascript the elements can be selected using document.getElementById(). 

const burger = document.getElementById('burger');
const nav = document.getElementById('nav-links');

And the list items can be accessed via the children attribute - though in order to iterate over them use the spread operator can be used to put the collection into an array:

[...nav.children].forEach((link, index) => {

The animation style on the list items can be moved to CSS - the only part that seems to need to stay in JavaScript is computing the animation delay of each list item based on the index.

The functionality of this line within the forEach in the click handler that is executed when link.style.animation is falsey

link.style.animation = `navLinkFade 0.5s ease forwards ${index / 7 + .3}s`;

Can be moved to CSS:

ul#nav-links.nav-active li {
    animation: navLinkFade 0.5s ease forwards;
}

This way the animation style only applies when the nav-active class is present on the unordered list.

Notice that the animation delay (i.e. ${index / 7 + .3}s) was removed from that style. For this, the forEach can be moved outside the click handler to set the animation delay property. 

[...nav.children].forEach((link, index) => {
    link.style.animationDelay = `${index / 7 + .3}s`;
});

Then the click handler doesn’t need to iterate through the list items - it just toggles class names. Hence the array of list items is only iterated through once instead of each time the burger menu is clicked.

A few of the CSS styles can be simplified - e.g.

Under .nav-links li the padding is:

 padding: 10px 10px 10px;

This can be simplified to 

padding: 10px;

The padding-left and padding-right rules could be simplified to a padding specification vertical | horizontal

e.g. under nav:

padding: 0 10%;

And similar for .landing p

A sibling selector could also be used to eliminate the need for the nav-active class on the ul. While it would require moving the <div class="burger"> before the unordered list, it would allow the CSS to be changed to this:

.burger.toggle + ul#nav-links {
  transform: translateX(0%);
}
.burger.toggle + ul#nav-links li {
  animation: navLinkFade 0.5s ease forwards;
}

And there is no need for that nav-active class.

This allows the click handler to be reduced to a single line:

burger.addEventListener('click', _ => burger.classList.toggle('toggle'));


const navSlide = () => {
  const burger = document.getElementById('burger');
  const nav = document.getElementById('nav-links');
  [...nav.children].forEach((link, index) => {
    link.style.animationDelay = `${index / 7 + .3}s`;
  });
  //togle nav
  burger.addEventListener('click', _ => burger.classList.toggle('toggle'));
}

navSlide();
@import url('https://fonts.googleapis.com/css?family=Montserrat:400,500,700&display=swap');
@import url('https://fonts.googleapis.com/css?family=Fira+Sans:400,400i,500,500i,700,700i&display=swap');
* {
  margin: 0;
  padding: 0;
}

body {
  background-color: #E5ECE9;
  height: 2000px;
  font-family: 'montserrat', sans-serif;
}

nav {
  display: flex;
  justify-content: space-between;
  align-items: center;
  min-height: 8vh;
  background-color: white;
  flex-basis: 1336px;
  padding-left: 10%;
  padding-right: 10%;
}

.logo img {
  height: 84px;
  max-width: 100%;
}

#nav-links {
  display: flex;
  justify-content: space-around;
}

#nav-links li {
  list-style-type: none;
  padding: 10px 10px 10px;
}

#nav-links a {
  color: black;
  text-decoration: none;
  letter-spacing: 2px;
  font-weight: 500;
  font-family: 'montserrat', sans-serif;
  font-size: 14px;
}

#nav-links #green-select {
  font-weight: bold;
  color: #A7E66E;
}

#burger {
  display: none;
  cursor: pointer;
}

#burger div {
  width: 25px;
  height: 3px;
  background-color: black;
  margin: 3px;
  transition: all 0.3s ease;
}

/* Responsive Design */

@media screen and (max-width: 1024px) {}

@media screen and (max-width:768px) {
  body {
    overflow-x: hidden;
  }
  #nav-links {
    position: absolute;
    right: 0px;
    height: 92vh;
    top: 9vh;
    background-color: white;
    display: flex;
    flex-direction: column;
    align-items: center;
    width: 100%;
    transform: translateX(100%);
    transition: transform 0.5s ease-in;
  }
  #nav-links li {
    opacity: 0;
  }
  #burger {
    display: block;
  }
}

#burger.toggle + ul#nav-links {
  transform: translateX(0%);
}
#burger.toggle + ul#nav-links li {
  animation: navLinkFade 0.5s ease forwards;
} 

@keyframes navLinkFade {
  from {
    opacity: 0;
    transform: translateX(50px);
  }
  to {
    opacity: 1;
    transform: translateX(0px);
  }
}

.toggle .line1 {
  transform: rotate(-45deg) translate(-5px, 6px);
}

.toggle .line2 {
  opacity: 0;
}

.toggle .line3 {
  transform: rotate(45deg) translate(-5px, -6px);
}
<nav>
  <div class="logo">
    <svg aria-hidden="true" class="native mtn1 svg-icon iconLogoSEAlternativeSm" width="107" height="15" viewBox="0 0 107 15"><path d="M0 3c0-1.1.9-2 2-2h8a2 2 0 012 2H0z" fill="#8FD8F7"></path><path d="M12 10H0c0 1.1.9 2 2 2h5v3l3-3a2 2 0 002-2z" fill="#155397"></path><path fill="#46A2D9" d="M0 4h12v2H0z"></path><path fill="#2D6DB5" d="M0 7h12v2H0z"></path></svg>

  </div>
  <div id="burger">
    <div class="line1"></div>
    <div class="line2"></div>
    <div class="line3"></div>
  </div>
  <ul id="nav-links">
    <li><a id="green-select" href="#">Showcase</a></li>
    <li><a href="#">About</a></li>
    <li><a href="#">Blog</a></li>
    <li><a href="#">Contact</a></li>
  </ul>
</nav>

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.