5
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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]);
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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] \$\endgroup\$ – Ahmed ALgrai Mar 2 '17 at 9:45
  • \$\begingroup\$ 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. \$\endgroup\$ – kamoroso94 Mar 2 '17 at 9:48
  • \$\begingroup\$ 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 \$\endgroup\$ – Ahmed ALgrai Mar 2 '17 at 9:55
  • \$\begingroup\$ Alright, I'll edit my answer. I hope it's what you're looking for so you can accept it :) \$\endgroup\$ – kamoroso94 Mar 2 '17 at 9:56
  • \$\begingroup\$ Great this is really getting better Thank You Very Much \$\endgroup\$ – Ahmed ALgrai Mar 2 '17 at 10:18
3
\$\begingroup\$

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]);
        }
    }
};
\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Ahmed ALgrai Mar 2 '17 at 8:15
  • \$\begingroup\$ @AhmedALgrai please have a look at the update. \$\endgroup\$ – Mario Santini Mar 2 '17 at 8:42
  • \$\begingroup\$ very nice ! I've just googled this trick and it blow my mind thanks i can tell that my code is getting cleaner now \$\endgroup\$ – Ahmed ALgrai Mar 2 '17 at 8:44

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.