JavaScript sidebar menu with subitems from a Json Object

Question

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.

Answer 1

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.