current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite
1

I want to make a nav that look like this:

<nav class="socialmediaicons">
   <ul>
     <li>
       <a>
        <img>
      </a>
    </li>
  </ul>
</nav>

Using the appendChild() method, and this is what I've done:

The myNode is the nav tag which has already been created via HTML.

var myNode = document.querySelector('.socialmediaicons');

var myEl = document.createElement('ul');

var myLi = document.createElement('li');

var myA = document.createElement('a');

var myImg = document.createElement('img');



var addEl = function() {
    var nodes = [myEl, myLi, myA, myImg];

    for(var i=0; i<nodes.length; i++) {
        myNode.appendChild(nodes[i]);

        for(var j=0; j<3; j++){
            nodes[j].appendChild(nodes[j+1]);
        }
    }

};

addEl();
console.log(myNode);

And it WORKS!!

I even added a src attribute to the myImg element using myImg.src= and the photo shows up. But is this code GOOD!?

Is there any better way to do it? (If you have a jQuery method it's OK, but show me the pure JavaScript method after it.) I like the pure JavaScript way and also for learning purposes.

share|improve this question
up vote 3 down vote

A better way is by using a template library like Handlebars.

The reason why it is better, stand on the fact you could separate logic code from your visualization code.

And more, using templates, you could have a closer look on what the page will became by looking to html code with the placeholders, that is easy to debug, instead of following complicated sequences of DOM calls.

I kindly suggest you to use such type of approach.

UPDATE

In response of your comment I'm extending my answer.

About your code, the issue I see is that you define an array of DOM element inside your function, but those elements are defined outside the scoop of your function.

This is like a nightmare to maintenance, and could easily be avoided.

Just pass the DOM object as the function arguments, and you have solved the issue:

var addEl = function(rootNode) {
    var nodes = Array.prototype.slice.call(arguments, 1);

    for(var i=0; i<nodes.length; i++) {
        rootNode.appendChild(nodes[i]);

        for(var j=0; j<3; j++){
            nodes[j].appendChild(nodes[j+1]);
        }
    }
};
share|improve this answer
    
i agree with you, and i'm not gonna do this in real life but like i've said its for learning purpose and I've seen it as a challenge on a video in Lynda and he get the same result using just javacript and he even said that there's no right way to do it and you could approached in different ways , but i feel like my way is not a best practice . @MarioSantini – Ahmed ALgrai 12 hours ago
    
@AhmedALgrai please have a look at the update. – Mario Santini 11 hours ago
    
very nice ! I've just googled this trick and it blow my mind thanks i can tell that my code is getting cleaner now – Ahmed ALgrai 11 hours ago
up vote 3 down vote

This is a good first attempt, but you have a couple things wrong with it. If you look at the for loops, you'll see it's adding each element to the nav in the same level (rather than nest them inside each other), but then proceeds to move it to the correct place with the inner for loop.

That's the second problem. You rebuild the nested element structure each time you iterate over the array. You should eliminate all this unnecessary work, and you can do it in all plain JavaScript without using jQuery.

Edit: changed function to accept any number of elements as arguments to nest (e.g., addEl(myNode, myEl, myLi, myA, myImg)).

function addEl() {
    var nodes = arguments;
    for(var i = 1; i < nodes.length; i++) {
        nodes[i - 1].appendChild(nodes[i]);
    }
}
share|improve this answer
    
definitely one of the most thing that I've noticed in the beginning is that i am taking the long road ! and you just did what I've exactly thought of but i didn't execute it well , i wonder can we combine your approach with what MarioSantini said about defining elements that is already defined outside [when we used the array] – Ahmed ALgrai 10 hours ago
    
You want to nest an arbitrary number of elements? You can do that, but I don't know why'd you want to generalize that. – kamoroso94 10 hours ago
    
what i want to say is to use the arguments trick instead of array i've tried this var nodes = Array.prototype.slice.call(arguments, 1); for(var i = 1; i < nodes.length; i++) { nodes[i - 1].appendChild(nodes[i]); } addEl(myNode, myEl, myLi, myA, myImg); but it did not work – Ahmed ALgrai 10 hours ago
    
Alright, I'll edit my answer. I hope it's what you're looking for so you can accept it :) – kamoroso94 10 hours ago
    
Great this is really getting better Thank You Very Much – Ahmed ALgrai 10 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.