1
\$\begingroup\$

I'm building this menu builder with JS. So far I have a simple version 1 implementation that does what I want but it needs improving. Couple of things that I'd like to improve:

  • Prevent unwanted input
  • Control input when there is margin for error, ie for the target field I'd like to have either a drop down or a checkbox.
  • Maintain state between page load (menu should remain on page after load)
  • Any code optimization

Here is the full code and a link to codepen:

// track the button clicks globally
var btnClicks = 0;

// create a function the append the form to the DOM on page load.
window.onload = createForm;

function createForm() {

  // initialize out form container
  var f = formContainer();

  // declare the variables we will use in one place
  var itemName, id, className, target, href, btn, text, h2;

  // create an array that will hold the values we wish to pass to our links
  // in the menu
  var listItem = [itemName, id, className, target, href];

  // Create the labels so each input can have a unique label
  var labels = ['Name', 'ID', 'Class', 'Target', 'Link'];

  // Helper text
  h2 = document.createElement('h2');
  text = 'Fill in the fields below to create a new menu item';
  h2.innerText = text;
  f.appendChild(h2);

  // loop through the list items
  listItem.forEach(function(element, index){
    // for each element, call the createDOMNode function and pass in required data
    element = createDOMNode(labels[index], 'input', 'text', 'input_' + index);
    // append each element to the form container
    f.appendChild(element);
  });

  // create the button and give it an onclick handler
  btn = document.createElement('button');
  btn.innerText = 'Create Menu Item';
  f.appendChild(btn);
  btn.onclick = getUserInput;
}

// get what the user inputted into the fields
function getUserInput() {
  // initialize some variables and an array
  var itemName, id, className, target, href;
  item = [];

  // access the values from the input fields
  values = [
    itemName = document.getElementById('input_0').value,
    id = document.getElementById('input_1').value,
    className = document.getElementById('input_2').value,
    target = document.getElementById('input_3').value,
    href = document.getElementById('input_4').value,
  ];

  // loop through each value
  values.forEach(function(element, index){
    // and push it into the item[] array
    if(element !== '') {
      item.push(element);
    }
  });

  // make sure required items are filled out
  if(values[0] === '' && values[4] === '') {
    alert('Name and Link are both required');
  } else if(values[0] === '') {
    alert('Name is required');
  } else if(values[4] === '') {
    alert('Link is required');
  }

  // if the array is not empty, then create the menu
  if(item.length !== 0) {
    createMenu(item);
  }

  // increase the button counter
  btnClicks += 1;

}
var nav = document.createElement('nav');
var ul = document.createElement('ul');
// function to create a new menu
function createMenu(item) {
  // create elements needed for menu

  var li = document.createElement('li');
  var a = document.createElement('a');
  var f = document.getElementById('formContainer');

  // trying to create the nav only on the first click
  if(btnClicks < 1) {
    nav.setAttribute('class', 'nav');
    document.body.insertBefore(nav, f);
    nav.appendChild(ul);
  }

  var arrayLength = item.length;
  // loop through items and set their attributes
  for(var i = 0; i < arrayLength; i++) {
    li.appendChild(a);
    a.innerText = item[0];
    a.setAttribute('id', item[1]);
    a.setAttribute('class', item[2]);
    a.setAttribute('target', item[3]);
    a.setAttribute('href', item[4]);

  }
  // and append them to the ul
  ul.appendChild(li);
}

function formContainer() {

  // create the element and set where it is displayed in the DOM
  var formContainer = document.createElement('div');
  formContainer.setAttribute('id', 'formContainer');
  var scriptTag = document.getElementsByTagName('script')[0];
  // document.body.insertBefore(formContainer, scriptTag);
  document.body.appendChild(formContainer);

  // style the element
  formContainer.style.width = '360px';
  formContainer.style.margin = '0 auto';
  formContainer.style.border = '1px solid #ddd';
  formContainer.style.padding = '15px';

  return formContainer;

}


function createDOMNode(label, element, type, id) {

  var l = document.createElement('label');
  l.innerText = label;

  // create the node and set it's type attribute
  var node = document.createElement(element);
  node.setAttribute('type', type);
  node.setAttribute('id', id);

  // style the node
  node.style.padding = '8px 4px';
  node.style.width = '100%';
  node.style.marginBottom = '10px';
  node.style.boxSizing = 'border-box';

  l.appendChild(node);

  return l;
}
\$\endgroup\$
3
1
\$\begingroup\$

createForm()

I don't think you need the listItem array or its named elements at all; you're not using it for anything. To build the form controls, you can loop through the labels array instead of the listItem array.

// loop through the list items
labels.forEach(function(element, index){
    // for each element, call the createDOMNode function and pass in required data
    // then append the result to the form container
    f.appendChild(createDOMNode(labels[index], 'input', 'text', 'input_' + index));
});

getUserInput()

You're missing the var keyword before defining the variable item.

There's no reason to define variables for itemName, id, className, target, and href, since you never reference those values by name. You can just populate your values array directly.

values = [
    document.getElementById('input_0').value,
    document.getElementById('input_1').value,
    document.getElementById('input_2').value,
    document.getElementById('input_3').value,
    document.getElementById('input_4').value,
];

consider objects with named properties as alternatives to arrays

In general, I think your code is over-using arrays to store values that are not inherently suited to that data structure.

A good example of this is the use of the values array and item array in getUserInput().

You're populating values with values from the text boxes, then looping through those values to check whether they're blank, and if not, you're then pushing those values into the item array to represent your actual item.

Consider instead capturing those values directly into an object, as below.

// access the values from the input fields
var item = {
    name: document.getElementById('input_0').value,
    id: document.getElementById('input_1').value,
    class: document.getElementById('input_2').value,
    target: document.getElementById('input_3').value,
    link: document.getElementById('input_4').value,
};
// make sure required items are filled out
if(item.name === '' && item.link === '') {
    alert('Name and Link are both required');
} else if(item.name === '') {
    alert('Name is required');
} else if(item.link === '') {
    alert('Link is required');
} else {  // if the array is not empty, then create the menu
    createMenu(item);
}

This saves you all the processing that would take place within the loop.

You would also need to update your createMenu() function to accommodate the new format for the item parameter.

// function to create a new menu
function createMenu(item) {
    // create elements needed for menu
    var li = document.createElement('li');
    var a = li.appendChild(document.createElement('a'));
    var f = document.getElementById('formContainer');

    // trying to create the nav only on the first click
    if(btnClicks < 1) {
        nav.setAttribute('class', 'nav');
        document.body.insertBefore(nav, f);
        nav.appendChild(ul);
    }

    a.innerText = item.name;
    a.setAttribute('id', item.id);
    a.setAttribute('class', item.class);
    a.setAttribute('target', item.target);
    a.setAttribute('href', item.link);
    ul.appendChild(li);
}

Notice also that you had an unnecessary loop in the createMenu() function which did the same thing 5 times in a row (set the inner text and attributes of the a element).

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the comments. Funny that I had actually already considered a lot of your points but wasn't sure how to do it the way you suggested. I'll give it try \$\endgroup\$ – Ryan Mc Dec 22 '16 at 19:56

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.