1
\$\begingroup\$

I'm kind of new to pure javascript and I wanted to try an exercise that I saw around the web. The exercise was to build from a json object (was not included in the exercise so I had to improvise ), a sidebar navigation menu with sub items.

With jQuery this would have been more easier, but truth is that I don't want to keep using jQuery just because it's easier.

I did this:

var nav = document.getElementById('navigation');

function linkHref(link, name){
  return ('<a href="'+ link +'">'+ name + '</a>');
}

function subsubmenu(parent, childdata){
  var submenumainlink = document.getElementById('submenu-' + parent.keyName);
  var submenu = document.createElement("ul");
  submenu.setAttribute('id', 'submenu-' + parent.keyName + '-menu');

  submenumainlink.appendChild(submenu);

  var submenuId =  document.getElementById('submenu-' + parent.keyName + '-menu');

  for(item of childdata){
    var line = document.createElement("li");
    line.className = item.submenu ? 'nav-item submenu': 'nav-item';
    line.innerHTML = linkHref(item.link, item.title)

    if(item.submenu){
        submenuId.appendChild(line);
        line.setAttribute('id', 'submenu-' + item.keyName);
        subsubmenu(item, item.submenu);
    }else{
      submenuId.appendChild(line);
    }
  }
}

function submenu(parent, childdata){
  var submenumainlink = document.getElementById('submenu-' + parent.keyName);
  var submenu = document.createElement("ul");
  submenu.setAttribute('id', 'submenu-' + parent.keyName + '-menu');

  submenumainlink.appendChild(submenu);

  var submenuId =  document.getElementById('submenu-' + parent.keyName + '-menu');

  for(item of childdata){
    var line = document.createElement("li");
    line.className = item.submenu ? 'nav-item submenu': 'nav-item';
    line.innerHTML = linkHref(item.link, item.title)

    if(item.submenu){
        submenuId.appendChild(line);
        line.setAttribute('id', 'submenu-' + item.keyName);
        subsubmenu(item, item.submenu);
    }else{
      submenuId.appendChild(line);
    }
  }
}

function createLink(data){
  for(navLink of data){
    var line = document.createElement("li");
    line.className = navLink.submenu ? 'nav-item submenu': 'nav-item';
    line.innerHTML = linkHref(navLink.link, navLink.title)
    if(navLink.submenu){
      //submenu(navLink, navLink.submenu);
      line.setAttribute('id', 'submenu-' + navLink.keyName);
      nav.appendChild(line);
      submenu(navLink, navLink.submenu);


    }else{
      nav.appendChild(line);
    }
  }
}

const navData = [
  {title: 'Game', link: '/games'},
  {title: 'Community',  link: '/community', keyName: 'community', submenu: [
    {title: 'Media',  link: '/media', keyName: 'media2', submenu: [
      {title: 'Barbarian',  link: '/barbarian'},
      {title: 'Demon Hunter',  link: '/demon-hunter'},
      {title: 'Monk',  link: '/monk'},
      {title: 'Witch Doctor',  link: '/witch-doctor'},
    ]},
    {title: 'Gameplay', link: '/games'},
    {title: 'Classes',  link: '/community', keyName: 'classes2', submenu: [
      {title: 'Barbarian',  link: '/barbarian'},
      {title: 'Demon Hunter',  link: '/demon-hunter'},
      {title: 'Monk',  link: '/monk'},
      {title: 'Witch Doctor',  link: '/witch-doctor'},
    ]},
    {title: 'Beta',  link: '/beta'},
    {title: 'Support',  link: '/support'}
  ]},
  {title: 'Forums', link: '/forums', keyName: 'forums', submenu: [
    {title: 'Media',  link: '/media', keyName: 'media', submenu: [
      {title: 'Barbarian',  link: '/barbarian'},
      {title: 'Demon Hunter',  link: '/demon-hunter'},
      {title: 'Monk',  link: '/monk'},
      {title: 'Witch Doctor',  link: '/witch-doctor'},
    ]},
    {title: 'Gameplay', link: '/games'},
    {title: 'Classes',  link: '/community', keyName: 'classes', submenu: [
      {title: 'Barbarian',  link: '/barbarian'},
      {title: 'Demon Hunter',  link: '/demon-hunter'},
      {title: 'Monk',  link: '/monk'},
      {title: 'Witch Doctor',  link: '/witch-doctor'},
    ]},
    {title: 'Beta',  link: '/beta'},
    {title: 'Support',  link: '/support'}
  ]},
  {title: 'Services',  link: '/services'},
];

createLink(navData);
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <meta http-equiv="X-UA-Compatible" content="ie=edge">
  <title>Document</title>
  <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/meyer-reset/2.0/reset.css" />
  <style>
    *{
      box-sizing: border-box;
    }
    body{
      font-family: Arial;
    }
    .nav-item{
      position: relative;
      max-width: 200px;
    }
    .nav-item a{
      display: block;
      background: #400000;
      color: orange;
      padding: 10px 15px;
      text-decoration: none;
      border: 1px solid orange;
      margin-top: -1px;
      max-width: 200px;
    }
    nav ul > .nav-item:first-child a{
      margin-top: 0;
    }
    .submenu > ul{
      display: none;
      position: absolute;
      left: 100%;
      margin-left: -1px;
      top: 0;
      width: 200px;
    }
    .nav-item.submenu:hover > ul{
      display: block;
    }
  </style>
</head>
<body>
  <nav>
    <ul id="navigation">
    </ul>
  </nav>
  <script type="text/javascript" src="nav.js"></script>
</body>
</html>

I made it work, but I'm sure this is pretty messy and ugly, or maybe not. Is there any way to keep it more simple? seems like a lot of useles lines. I been trying to find documentation about how to do this kind of things, but they always suggest using jQuery.

\$\endgroup\$
1
\$\begingroup\$

Don't repeat yourself

The functions submenu and subsubmenu look identical to me. And if I overlooked a difference, I wouldn't blame myself. I suspect you copy-pasted the original and then forgot to make the necessary changes. But the copy-pasting step is a mistake. Instead of copy-pasting, consider extracting common behavior to a helper function.

I see code duplication at smaller scales too, for example here:

if(item.submenu){
    submenuId.appendChild(line);
    line.setAttribute('id', 'submenu-' + item.keyName);
    subsubmenu(item, item.submenu);
}else{
  submenuId.appendChild(line);
}

Both branches of the condition have submenuId.appendChild(line);, and since it appears at the beginning of both branches, it should be moved out of the conditional:

submenuId.appendChild(line);

if (item.submenu) {
    line.setAttribute('id', 'submenu-' + item.keyName);
    subsubmenu(item, item.submenu);
}

Manipulating DOM objects

Instead of setting innerHTML like this:

line.innerHTML = linkHref(item.link, item.title)

It would be more natural to add as a child:

line.appendChild(linkHref(item.link, item.title))

Similarly, instead of stitching together an <a/> element from string pieces like this:

return ('<a href="'+ link +'">'+ name + '</a>');

It would be more idiomatic to work with DOM objects:

function link(path, title) {
    var link = document.createElement('a');
    link.href = path;
    link.text = title;
    return link;
}

I also renamed the function to something more natural than linkHref, and its parameters to more accurately reflect what they represent.

\$\endgroup\$

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.