3
\$\begingroup\$

I've got a foreach loop, going through each section of an XML document (an example of said XML document can be found in this SO question) and only display them in divs with corresponding classes.

<xsl:for-each select="sections/section">
  <xsl:sort select="sc_index" />
  <h2>
    <xsl:value-of select='name' />
  </h2>
  <div class="rel10r3">
    <ul>
      <xsl:for-each select="instructions/instruction[releases//release='10r3']">
        <xsl:sort select="index" />
        <li>
          <xsl:value-of select="content" />
        </li>
      </xsl:for-each>
    </ul>
  </div>
  <div class="rel11r2" style="display:none">
    <ul>
      <xsl:for-each select="instructions/instruction[releases//release='11r2']">
        <xsl:sort select="index" />
        <li>
          <xsl:value-of select="content" />
        </li>
      </xsl:for-each>
    </ul>
  </div>
  <div class="rel12r1" style="display:none">
    <ul>
      <xsl:for-each select="instructions/instruction[releases//release='12r1']">
        <xsl:sort select="index" />
        <li>
          <xsl:value-of select="content" />
        </li>
      </xsl:for-each>
    </ul>
  </div>
  <div class="rel12r2" style="display:none">
    <ul>
      <xsl:for-each select="instructions/instruction[releases//release='12r2']">
        <xsl:sort select="index" />
        <li>
          <xsl:value-of select="content" />
        </li>
      </xsl:for-each>
    </ul>
  </div>
  <div class="rel13r1" style="display:none">
    <ul>
      <xsl:for-each select="instructions/instruction[releases//release='13r3']">
        <xsl:sort select="index" />
        <li>
          <xsl:value-of select="content" />
        </li>
      </xsl:for-each>
    </ul>
  </div>
</xsl:for-each>

What I do after that is using a drop-down menu, and with the help of javascript switch the style.display property of the divs accordingly.

Form:

<form>
  <select name="release" onchange="return showHide(this);">
     <option value="10r3" selected="selected">10r3</option>
     <option value="11r2">11r2</option>
     <option value="12r1">12r1</option>
     <option value="12r2">12r2</option>
     <option value="13r1">13r1</option>
   </select>
 </form>

Javascript:

function showHide(selection) {
    var f = selection.form;
    // Retrieve the value of the option selected.
    var opt = selection.options[selection.selectedIndex].value;
    /* Retrieve all the div elements of the document and then parse them to
     * see if they have a classname corresponding with the selected release.
     * If it is the case, remove all display style, otherwise set the
     * display style to none.
     */
    var divArray = document.getElementsByTagName("div");
    for(var i=0; i<divArray.length; i++){
        var relArray = ["10r3","11r2","12r1","12r2","13r1"];
        for(var j=0; j<relArray.length; j++){
            var relName = relArray[j];
            if(divArray[i].className == "rel"+relName){
                if(opt===relName){
                    divArray[i].style.display = "";
                }else{
                    divArray[i].style.display = "none";
                }
            }
        }
    }
    return false;
}

I've already refactored my javascript code, but there is still plenty of repetitive code out there, and it can only grow the more releases there will be.

EDIT: One more thing, those instruction are shown in a CSS pop-up using a technique I found (reference):

HTML:

<div>
  <a id="helplink" href="#" onclick="popup('popUpDiv')">Click Here to Open the Help</a>
</div>

Javascript:

function toggle(div_id) {
    var el = document.getElementById(div_id);
    if ( el.style.display == 'none' ) { el.style.display = 'block';}
    else {el.style.display = 'none';}
}
function blanket_size(popUpDivVar) {
    if (typeof window.innerWidth != 'undefined') {
        viewportheight = window.innerHeight;
    } else {
        viewportheight = document.documentElement.clientHeight;
    }
    if ((viewportheight > document.body.parentNode.scrollHeight) && (viewportheight > document.body.parentNode.clientHeight)) {
        blanket_height = viewportheight;
    } else {
        if (document.body.parentNode.clientHeight > document.body.parentNode.scrollHeight) {
            blanket_height = document.body.parentNode.clientHeight;
        } else {
            blanket_height = document.body.parentNode.scrollHeight;
        }
    }
    var blanket = document.getElementById('blanket');
    blanket.style.height = blanket_height + 'px';
    var popUpDiv = document.getElementById(popUpDivVar);
    popUpDiv_height=50;
    popUpDiv.style.top = popUpDiv_height + 'px';
}
function window_pos(popUpDivVar) {
    if (typeof window.innerWidth != 'undefined') {
        viewportwidth = window.innerHeight;
    } else {
        viewportwidth = document.documentElement.clientHeight;
    }
    if ((viewportwidth > document.body.parentNode.scrollWidth) && (viewportwidth > document.body.parentNode.clientWidth)) {
        window_width = viewportwidth;
    } else {
        if (document.body.parentNode.clientWidth > document.body.parentNode.scrollWidth) {
            window_width = document.body.parentNode.clientWidth;
        } else {
            window_width = document.body.parentNode.scrollWidth;
        }
    }
    var popUpDiv = document.getElementById(popUpDivVar);
    window_width=50;
    popUpDiv.style.left = window_width + 'px';
}
function popup(windowname) {
    blanket_size(windowname);
    window_pos(windowname);
    toggle('blanket');
    toggle(windowname);     
}

CSS:

#blanket {
background-color:#111;
opacity: 0.65;
filter:alpha(opacity=65);
position:absolute;
z-index: 9001;
top:0px;
left:0px;
width:100%;
}
#popUpDiv {
position:absolute;
background-color:#eeeeee;
width:700px;
height:700px;
z-index: 9002;
}

I mention this information, because the refactoring proposed seems to mess with my pop-up.

EDIT2: One of the shortcoming of the actual code I noticed is that I wouldn't be able to use release element of value All, which would make that the parent instruction keep its place relative to other elements of a specific release, whichever release is selected.

\$\endgroup\$

3 Answers 3

2
\$\begingroup\$
  1. in your showHide() function:

    var relArray = ["rel10r3","rel11r2","rel12r1","rel12r2","rel13r1"];
    for(var i=0; i<divArray.length; i++)
    {
       var currentDiv = divArray[i];
       if(currentDiv.className && relArray.indexOf(currentDiv.className) != -1)
       {
            currentDiv.style.display = "";     
       }
       else
       {
            currentDiv.style.display = "none";
       }
    }
    
  2. Don't be skimpy on the formatting as it makes a big difference on the readability. forexample:

    function toggle(div_id) {
        var el = document.getElementById(div_id);
        if (el.style.display == 'none' )
        {
            el.style.display = 'block';
        }
        else 
        {
            el.style.display = 'none';
        }
    }
    
  3. Javascript is very nice with falsey values:

    if (typeof window.innerWidth != 'undefined') {
        viewportheight = window.innerHeight;
    } else {
        viewportheight = document.documentElement.clientHeight;
    }
    

    can be written:

    if (window.innerWidth) {
        viewportheight = window.innerHeight;
    } else {
        viewportheight = document.documentElement.clientHeight;
    }
    

    or

    viewportheight = window.innerHeight || document.documentElement.clientHeight;
    
  4. patterns like:

    if(){
        //
    }else{
        if(){
            //
        }else{
            //
        }
    }
    

    can be written much easier and cleaner:

    if(){
        //
    }else if(){
        //
    }else{
        //
    }
    
  5. you have:

    if ((viewportwidth > document.body.parentNode.scrollWidth) && (viewportwidth > document.body.parentNode.clientWidth)) {
         //etc etc
    

    can be:

    window_width = Math.max(viewportwidth, document.body.parentNode.clientWidth, document.body.parentNode.scrollWidth); 
    
  6. you are using variables to hold a value for one line:

    var popUpDiv = document.getElementById(popUpDivVar);
    window_width=50;
    popUpDiv.style.left = window_width + 'px';
    

    instead:

    var popUpDiv = document.getElementById(popUpDivVar);
    if(popUpDiv) // incase it couldn't find the div.
    {
        popUpDiv.style.left = "50px";
    }
    

TL;DR? http://jsfiddle.net/37Uge/1/

\$\endgroup\$
6
  • \$\begingroup\$ Thanks for all the tip. To be honest, the css pop-up code aren't from me, I added the source in my post. But about the first point, it doesn't really work. When the call the function showHide is triggered, all the contents inside divs disapear, included my drop-down menu. When I choose to move the relArray.indexOf check outside of the if else structure, so then, when the function is called, all the divs are displayed. \$\endgroup\$
    – Eldros
    Commented Oct 6, 2011 at 14:07
  • \$\begingroup\$ @Eldros Thats because i didn't write the relArray properly I only put 'rel' in the first element instead of all of them. I'll fix it and you can try it again. \$\endgroup\$ Commented Oct 6, 2011 at 22:37
  • \$\begingroup\$ @Eldros about your edit2 you could put a specific value (e.g. 99) and test for this before looping. \$\endgroup\$ Commented Oct 6, 2011 at 22:45
  • \$\begingroup\$ I already caught your mistake and put rel before each release ids before I made my last comment, so it is not the source of the problem. About using a specific value, I'll see what I can do. \$\endgroup\$
    – Eldros
    Commented Oct 7, 2011 at 6:53
  • 1
    \$\begingroup\$ After a few fiddling I came to a working code: link \$\endgroup\$
    – Eldros
    Commented Oct 7, 2011 at 7:43
1
\$\begingroup\$

I'd cache your loop variables as well...

 for(var i=0; i<divArray.length; i++){}

...would be written

var len = divArray.length;

for(var i = 0; i < len ; i++){}

This is according to Addy Osmani's blog post "Lessons From A JavaScript Code Review":

Problem: An uncached array.length is being used in all for loops. This is particularly bad as you're using it when iterating through 'HTMLCollection's

\$\endgroup\$
1
  • \$\begingroup\$ a shorter way of writing this would be: for(var i = 0, len = divArray.length; i < len ; i++) \$\endgroup\$ Commented Oct 5, 2011 at 2:15
0
\$\begingroup\$

On the js side, I'm not convinced you need the double looping. Have you tried something like this:

var optClass = "rel" + selection.options[selection.selectedIndex].value;
var divArray = document.getElementsByTagName("div");
for (var i = 0; i < divArray.length; i++){
    divArray[i].style.display = optClass === divArray[i].className ? "" : "none";  
}

On the xslt side, you will want to factor the div blocks in a separate template with parameters for the release number and the style.

\$\endgroup\$
3
  • \$\begingroup\$ Sorry, but your refactoring broke the function, so I had to come back to my code. \$\endgroup\$
    – Eldros
    Commented Sep 16, 2011 at 12:39
  • \$\begingroup\$ Oops, messed up array iteration syntax :-). I updated the code. \$\endgroup\$ Commented Sep 16, 2011 at 14:15
  • \$\begingroup\$ Now, when I change an option in my drop down menu, the css pop-up disappears completely. I suspect that it hide also the div containing my help link. \$\endgroup\$
    – Eldros
    Commented Sep 20, 2011 at 6:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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