views:

47

answers:

6

I have a basic show/hide javascript that works, as long as i don't make it dynamic and make sure of a parameter. I would appreciate a lot if anyone could help me figure out why the dynamic version doesn't work.

Working code:
javascript

function togglesDiv(){  
  var catdiv = document.getElementById("addNewCat");  
  if(catdiv.style.display == ""){  
    catdiv.style.display = "none";  
  } else {  
    catdiv.style.display = "";  
  }  
}  

html

<span onclick="togglesDiv();">Add new category</span>  
<div id="addNewCat" style="display: none;">  
lalala  
</div>

Non working code:
javascript

function togglesDiv(divsId){  
  var catdiv = document.getElementById("divsId");
  if(catdiv.style.display == ""){  
    catdiv.style.display = "none";  
  } else {  
    catdiv.style.display = "";  
  }  
}  

html

<span onclick="togglesDiv(addNewCat);">Add new category</span>  
<div id="addNewCat" style="display: none;">  
lalala  
</div>
+5  A: 

You have a variable name wrapped in string delimiters, making it a string literal instead of a variable. Change

var catdiv = document.getElementById("divsId");

To

var catdiv = document.getElementById(divsId);

On the flipside, the call to the function needs the quotes in it's argument (because it should be a string), you can use single quotes to avoid confliction:

<span onclick="togglesDiv('addNewCat');">Add new category</span>  
Andy E
This is the correct answer. When you add the quotes around divsId, it makes it a string with a value divsId, meaning it is looking for a div with that id. Without the quotes, it will look for the value of the string variable named divsId
Barlow Tucker
Doh :)Thanks, and thanks everyone else too :)
Poppe76
A: 

Remove the quotes:

var catdiv = document.getElementById("divsId");

Becomes

var catdiv = document.getElementById(divsId);

You don't have an element with an ID of "divsId".

On a completely unrelated note, you can't be sure that catdiv.style.display will always be equal to "" when it is visibile. There are other styles which cause it to be displayed ('inline', 'block', for example).

A better solution might be:

function togglesDiv(divsId){  
  var catdiv = document.getElementById("divsId");
  if(catdiv.style.display === "none"){  
    catdiv.style.display = "";  
  } else {  
    catdiv.style.display = "none";  
  }  
}  

(And yes, I did mean to put the triple equals === in)

James Wiseman
Remember that *element.style.display* is equivalent to inline styles and isn't affected by CSS rules. If you're in complete control of the script and the markup, you can be fairly certain that `element.style.display == ""` if you haven't already modified it. That being said, it is better to check against *"none"* than *""*.
Andy E
A: 

Your code is looking for a div with an ID "divsId" change your code to:

   function togglesDiv(divsId){  
      var catdiv = document.getElementById(divsId);
      if(catdiv.style.display == ""){  
        catdiv.style.display = "none";  
      } else {  
        catdiv.style.display = "";  
      }  
    } 
JKirchartz
A: 

Because you are looking for a div called "divsId" rather than the value in the variable divsId. Remove the speach marks!

Woody
A: 

Remove quotes from var catdiv = document.getElementById("divsId"); should be var catdiv = document.getElementById(divsId);

And add quotes:

<span onclick="togglesDiv(addNewCat);">Add new category</span>

Should be

<span onclick="togglesDiv('addNewCat');">Add new category</span>  
giker
A: 

Improved function

function toggleDisplay(id){
    var el = document.getElementById(id);
    if(!el) return true;
    // feature detect to support IE versions.
    var dis = 'currentStyle' in el ? el.currentStyle.display : el.style.display;
    // toggle display
    el.style.display = /none/i.test(dis) ? '' : 'none';
    // prevent memory leak
    el = null;
}

And as mentioned, quotes are needed when writing yucky inline javascript.

<span onclick="toggleDisplay('addNewCat')"> ... etc

Tbh. pick a js toolkit/library and use it over reinventing the wheel yourself and stop writing inline javascript, your life and happiness will improve substantially if you do =P

BGerrissen
*currentStyle* and *style* aren't the same, so you'll run into inconsistencies between browsers. *window.getComputedStyle()* is the standards alternative to *currentStyle*.
Andy E
ya i use jquery mostly otherwise, but this was just such a small thing in a personal backend system so i didn't see the point in using something bigger.
Poppe76