DOM building using DocumentFragment

Question

I want to build the markup below by iterating on an array of objects. The structure of the object is also listed below. I need some tips on how to make it clean.

Markup I want to build:

<!--the markup I want to build-->
<div class="pull-left col-xs-4">
    <div class="thumbnail">
        <div class="caption">
            <h5>Crowne Plaza Hotel</h5>
        </div>
        <img class="img-responsive" src="img/crown-plaza.jpg" alt="Crowne Plaza Hotel">
        <div class="caption">
            <h3 class="pull-right">
                <small>
                    <s>S$ 275</s>
                </small>S$ 275</h3>
            <div class="clearfix"></div>
        </div>
    </div>
</div>

JavaScript:

//the object structure of each data
{
    name: "Hotel A",
    loc: "Prontera Rd",
    dist: 9,
    img: "img/rendezvous-hotel.jpg",
    price: 350,
    deal_price: 350,
    currency: "S$",
    star_rating: 4
}

function constructDOMFragment(data) {
    var fragment = document.createDocumentFragment();
    for (var i = data.length - 1; i >= 0; i--)(function(obj) {

        var div_container = document.createElement("div");
        div_container.className = "pull-left col-xs-4";

        var div_thumbnail = document.createElement("div");
        div_thumbnail.className = "thumbnail";

        div_container.appendChild(div_thumbnail);

        var div_caption_top = document.createElement("div");
        div_caption_top.className = "caption";

        var h5 = document.createElement("h5");
        h5["innerHTML" || "textContent"] = obj.name;
        div_caption_top.appendChild(h5);
        div_thumbnail.appendChild(div_caption_top);

        var img = document.createElement("img");
        img.className = "img-responsive";
        img.src = obj.img;
        img.alt = obj.name;

        div_thumbnail.appendChild(img);

        var div_caption_bottom = document.createElement("div");
        div_caption_bottom.className = "caption";


        var h3_deal_price = document.createElement("h3");
        h3_deal_price.className = "pull-right";
        if (obj.price !== undefined && obj.price && obj.price != obj.deal_price) {
            var price = document.createElement("small");
            var strikethrough = document.createElement("s");
            strikethrough["innerHTML" || "textContent"] = obj.currency + " " + obj.price;
            price.appendChild(strikethrough);
            h3_deal_price.appendChild(price);
        }
        h3_deal_price.appendChild(document.createTextNode("  " + obj.currency + " " + obj.price));

        div_caption_bottom.appendChild(h3_deal_price);

        var clearfix = document.createElement("div");
        clearfix.className = "clearfix";
        div_caption_bottom.appendChild(clearfix);

        div_thumbnail.appendChild(div_caption_bottom);
        fragment.appendChild(div_container);

    })(data[i]);

    var clearfix = document.createElement("div");
    clearfix.className = "clearfix";
    fragment.appendChild(clearfix);

    return fragment;
};

Answer 1

Interesting question,

• As @rotora mentioned, h5["innerHTML" || "textContent"] cannot work, you are not testing cross browser, you would have caught this otherwise
• Naming, div_caption_bottom is an unfortunate name for 3 reasons
  • This should be lowerCamelCase, so divCaptionBottom
  • The tag type div is an implementation detail, are you going to rename this to span CaptionBottom is you starting using spans? I would rather see CaptionBottom
  • I was lying, bottomCaption ( adjectiveNoun ) reads much better

• From a 'Dont Repeat Yourself' perspective, you are mostly doing

  • Creating elements with a tag type
  • Assigning class names
  • Adding elements to other elements

  If you were to use a function like this (untested, but you should grasp where this is going):

  function createStyledElement( tag , className ){
    var element = document.createElement( tag );
    element.className = className;
    element.addStyledElement = createStyledElement;
  
    if( this instanceof HTMLElement )
    {
      this.appendChild( element );
    }
  
    return element;
  }
  

  Then your code would look more like this:

  var container = createStyledElement( 'div' , 'pull-left col-xs-4' );
  var thumbnail = container.addStyledElement( 'div' , 'thumbnail' );
  var captionTop = thumbnail.addStyledElement( 'div' , 'caption' );
  var header5 = captionTop.addStyledElement( 'h5' , 'caption' );
  header5.innerHTML = header5.innerHTML = obj.name;
  
  var img = thumbnail.addStyledElement( 'img' , 'img-responsive' );
  img.src = obj.img;
  img.alt = obj.name;
  
  var captionBottom = thumbnail.addStyledElement( 'div' , 'caption' );
  

• From a performance perspective, the speed of building HTML through templates or strings changes over time and is different per browser. You should check out templating libraries, but possibly your code is the right way to go. It's hard to tell.

• Also from a performance perspective, creating functions in a loop, is a no-no

• Finally, jshint.com found almost nothing to complain about besides some missing semicolons.

Minor update on templating

From your comment, you might be overestimating the effort templating takes.

If you define your template like this in your HTML:

<div id="hotelTemplate" style="display:none">
<!--the markup I want to build-->
<div class="pull-left col-xs-4">
    <div class="thumbnail">
        <div class="caption">
            <h5>~</h5>
        </div>
        <img class="img-responsive" src="~" alt="~">
        <div class="caption">
            <h3 class="pull-right">
                <small>
                    <s>~</s>
                </small>~</h3>
            <div class="clearfix"></div>
        </div>
    </div>
</div>
</div>

And then use for example this simple template filler

function fillTemplate( s )
{ //Replace ~ with further provided arguments
  for( var i = 1, a = s.split('~'), s = '' ; i < arguments.length ; i++ )
    s = s + a.shift() + arguments[i];
  return s + a.join("");
}   

Then you can do something like this:

var template = document.getElementById('hotelTemplate').innerHTML;
var html = fillTemplate( template , obj.name , obj.img , obj.name , obj.deal_price , obj.price );

var div = document.createElement( 'div' );
div.innerHTML = html;
fragment.appendChild( div );

Answer 2

There are several problems in your code:

---

What is r.price?

---

h5["innerHTML" || "textContent"] = obj.name makes no sense. You could just a well write h5["innerHTML"] = obj.name. Your code won't test if such properties exist.

Even h5["innerHTML"] || h5["textContent"] won't do much good, since that just returns the value of either property, so you can't use it to set it.

You'd need to use something like

if (typeof h5.innerHTML == "undefined") { // no need for bracket notation here any more
  h5.innerHTML = obj.name
} else {
  h5.textContent = obj.name
}

However even that isn't very nice, since the element may have innerHTML set even if the browser doesn't actually use it.

Also innerHTML and textContent are "incompatible" to each other. For example, if you would assign the string "<i>Example</i>" to them, then using innerHTML will parse the HTML and display the text Example (in italic), while textContent will display the literal text <i>Example</i>.

The cleanest way would be to create and append explicit text nodes.

---

I don't see any benefit in using a function as the body of the for loop.

---

Generally building a fragment like this isn't very practical. For one it's terribly slow. For the other it's very difficult to read and makes changing the HTML structure a real bother, especially since it contains presentational information, such as Bootstraps grid classes and the clearfix elements (which IMHO are usually unnecessary nowadays).

Consider using a template mechanic of some kind. Either build something simple yourself, or use one of the plenty JavaScript template libraries, such as mustache.