current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
2

I'm new to jQuery, and am using it to dynamically construct chunks of a page.

The created HTML is what I want, but I'm not sure if the way I'm generating it is sensible or idiomatic.

function setupIndex()
{
  $(document.body).append($("<img/>", {src : 'logo.png'}));
  var div = $("<div/>", {"class" : "content"}).appendTo(document.body);

  div.append("<h2/>", { text :'My Panels'});
  div.append($("<table/>", {id : 'maintable'}));
}

function addPanel(name, desc)
{
  if (!$("#maintable"))
    setupIndex();

  var row = $("<tr/>",{"class":"panels"}).appendTo("#maintable");

  var div = $("<td/>").appendTo(row);
  div.append($("<h3/>").append($("<a/>", { text : name, href : name + "/index.html"})));
  div.append(document.createTextNode(desc));

  div = $("<td/>").appendTo(row);

  var anc = div.append($("<a/>", {href: name + "/index.html"}));
  anc.append($("<img/>", { css : {width : "200px", height : "auto"}, src : name + "/thumbnail.png"}));
}
share|improve this question

1 Answer 1

up vote 5 down vote accepted

I know that the following might seem crazy at first but humour me for a minute. The main improvement is so that variables are not reused (which confuses their meaning) and the subtle .appendTo("#maintable") is at the end (which means the document is altered once).

There is not functional need to store the objects into variables as they aren't re-used. I often try to write in this pattern as it looks more like the way you'd write html. 

if (!$("#maintable"))
    setupIndex();

$("<tr/>", {"class":"panels"})
    .append(
        $("<td/>")
            .text(desc)
            .prepend(
                $("<h3/>")
                    .append($("<a/>", { text : name, href : name + "/index.html"})))
            )
    )
    .append(
        $("<td/>")
            .append($("<a/>", {href: name + "/index.html"}))
            .append($("<img/>", { css : {width : "200px", height : "auto"}, src : name + "/thumbnail.png"}))
    )
    .appendTo("#maintable");
share|improve this answer
    
I've just found a StackOverflow question similar: stackoverflow.com/questions/9760328/… – James Khoury Dec 4 '13 at 2:52
1  
Your first conditional won't work as expected: $() always returns a jQuery object (i.e. something truth'y) regardless of whether it finds an element, so setupIndex() will never be called. You'll want to check if($('#maintable').length === 0) (or, shorter, if(!$('#maintable').length)) – Flambino Dec 4 '13 at 13:23
    
@Flambino. Yup, you're right. I hadn't spotted that. – Roddy Dec 4 '13 at 21:07
    
@JamesKhoury - Thanks. What about $("<td/>") vs. $("<td>") ? Either seems to work... – Roddy Dec 4 '13 at 21:10
1  
@Roddy I am not sure jQuery sees any difference between $("<td/>") and $("<td>") – James Khoury Dec 5 '13 at 0:49

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.