1
\$\begingroup\$

I am looking for someone to review my solution to a jQuery DOM manipulation exercise on appendto.com: http://learn.appendto.com/lesson/dom-manipulation-101#exercise

I'm wondering if there is a way I can optimize or if there are other aspects I need to consider when building a solution like this.

The problem is outlined in the fiddle: http://jsfiddle.net/KhwZS/1283/

Here is my solution:

var getFirstTwoWords = function(str) {
    return str.split(/\s+/).slice(1,3).join(" ");
}

var tabLinks = $("<ul></ul>");
$("div.tab").each( function( idx ) {
    var id = "tabs-" + (idx + 1);

    var text = $(this)
        .removeClass("tab")
        .attr("id", id)
        .text();

    var tabLabel = getFirstTwoWords(text);

    // Create corresponding tab link
    tabLinks.append("<li><a href=\"#" + id + "\">" + tabLabel + "</a></li>");  
});

$(".tabs div:first").before(tabLinks);
$( ".tabs" ).tabs();

Optimized version:

var tabLinks = $("<ul></ul>");
$("div.tab").each( function( idx ) {
    var id = "tabs-" + (idx + 1);

    var tabLabel = $(this)
        .removeClass("tab")
        .attr("id", id)
        .find("h3")
        .remove()
        .text();

    // Create corresponding tab link
    tabLinks.append("<li><a href=\"#" + id + "\">" + tabLabel + "</a></li>");  
});

$(".tabs")
   .prepend(tabLinks)
   .tabs();
\$\endgroup\$
1
\$\begingroup\$

Your code's very nice. My points below are mostly of the "but what if..." variety concerning the DOM manipulation, rather than the JavaScript itself. In other words, your solution seems spot on for the given exercise, whereas my comments mostly refer to situations which are not part of the exercise, but could arise in the real world.

Aside: The exercise could be clearer in its naming. The container element (singular) has a class called tabs (plural), while the DIVs (plural) within it have a class called tab (singular). It's sort of logical, but it gets confusing nonetheless. And I say "sort of logical", because the .tab elements aren't actually tabs - they're "panels" in jQuery UI's terminology. Hence, their class name should really be "panel". But I digress...

First off, what would happen if the page contains several "tabable" sections? The fact that .tabs is a class name - not an ID - suggests that there might be multiple div.tabs container elements, each with their own set of div.tab elements.

Well, with your code, things go a little awry (see here): Both tab containers get tab links for all the panels on the page, even those belonging to another container.

The trick would be to go through each div.tabs (i.e. tab container) element in turn, and manipulate their individual structures one by one:

$("div.tabs").each(function (containerIndex) {
  var container = $(this),
      tabLinks  = $("<ul></ul>");

  container.find("div.tab").each(function (tabIndex) {
    var id = "tabs-" + containerIndex + "-panel-" + tabIndex; // more precise ID

    // your code here ...
  });

  container
    .prepend(tabLinks)
    .tabs();
});

See the result here

Second, what might happen if an <h3> heading has HTML in its text or has its own child elements? Here are examples of each of those cases:

  • Literal HTML: <h3>When to use &lt;div&gt; elements</h3>
  • HTML formatting: <h3>When to use <code>div</code> elements</h3>

Here's the result, using your code

Now, this is something where there may not be "one right answer" - it all depends on the content you're working with. However, it's worth considering.

In your case, you use .text() to get the text out of the heading, but you don't use .text() to put that text into the the link; you "manually" create a link by concatenating strings.
So, in the first case, the string you get out is When to use <div> elements. Notice that the entity-encoded < and > have become plain characters. When you then concatenate that with other strings of HTML to make the link, you end up more HTML than you intended; there's an unclosed DIV in the middle of your link!
In the second case, .text() completely ignores the <code> tag. That doesn't break the link, but you lose the formatting and semantics of the <code> tag, which - one might assume - was put there for a reason by whoever wrote the heading. Since you also remove the original <h3>, that formatting is now nowhere to be found.

I said there was no right answer, because to fix the first case, you'd need to use .text() to extract and insert the heading's content, as it prevents the string from being interpreted as HTML. But to fix the other case, you'd need to use .html() instead, to ensure the string is interpreted that way. Now, using .html() would actually solve both cases (preserving already-encoded entities and tags), but the next question is whether you actually want that. What if the heading itself contains a link? Then you'll end up with a link inside a link... This of course doesn't apply only to tabs and headings, but to any situation where you're extracting and inserting content, which is why I brought it up.

But again, I'm just trying to poke holes in things here, so don't let it discourage you. It's more to illustrate the sort of things you might run in to. The possible solutions I can think are outside the scope of this answer though (it'd involve some different approaches), and very much outside the scope of the original exercise.

Speaking of "out of scope", I've rambled plenty here, so I'll stop.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Nice explanation +1 for it :) \$\endgroup\$ – Owais Qureshi May 10 '13 at 22:07

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.