JavaScript Menu Builder

Question

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;
}

• BCdotWEB: Please note that questions asking for advice about code not yet written are off-topic. Please follow the tour and read "How do I ask a good question?", "What topics can I ask about here?" and "What types of questions should I avoid asking?".
• Ryan Mc: Sorry. I thought it was complete. It's a complete version 1...
• BCdotWEB: I'm referring to things like "I'd like to have" etc. Right now it sounds like you want us to provide things like a solution to "Maintain state between page load" etc.

Answer 1

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).

• Ryan Mc: 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