JS innerHTML function not behaving

Why doesn't this script do what I think it should?

[droverholidays]

Hi all

I'm trying to set up a function whereby on clicking a link, another part of the page will show changed text (there will be several such links, and I only want one option - i.e. the most recent click - showing at any one time). If I understand rightly there are two ways to do this - (1) create <divs> with display:none; and change this to display:block; when the link is clicked or (2) using innerHTML. I've been trying to use (2) since I couldn't figure out a way to set the first div back to display:none; when the second link is clicked and so on.

Here's what I've got so far:

The script (in the head section):

<script type="text/javascript">
function changeText(){
var details = document.getElementById('details').value;
document.getElementById('showdetails').innerHTML = details;
}
</script>

The div I want to change:

<span id="showdetails">Text I want to change</span>

The link:

<li><a id="details" value="What I want to change it to" onclick="javascript:changeText(); ">Link Text</a>

The onclick also contains some other javascript, which I've removed for clarity. The other (two) functions are separated by ; (from this one and from one another) and work fine both before I add this one and when it's there.

Result:

When I click the link (in FF) the div I'm trying to change changes to "undefined".

Can anyone see where I'm going wrong please? Bearing in mind I am just getting started with JS.

[droverholidays]

PS if there is a way to do this using display:none; and display:block; (i.e. how to make the previous div go back to hidden when the user clicks the next link) I'd be interested in how that works too!

[chasehx]

sure.

document.getElementById('showdetails').style.display = "none";

On the flip replace none with 'block'

[droverholidays]

Thanks - I'm not sure I get it though. Wouldn't this mean having a separate script for each link (to display a different div for each one)? And how would it know which div to hide when you clicked a different link?

[Fotiman]

You should create your page with the assumption that JavaScript is not available, and then use progressive enhancement to add the functionality of toggling 1 div at a time. This will allow your site to continue to function for users (and search engines) that don't have JavaScript enabled. So your markup should be something like:

<ul>
  <li><a href="#item1">Item 1</a></li>
  <li><a href="#item2">Item 2</a></li>
  <li><a href="#item3">Item 3</a></li>
</ul>
<div id="item1">Item 1 content</div>
<div id="item2">Item 2 content</div>
<div id="item3">Item 3 content</div>

Next, you want to enhance the page to toggle only 1 of the divs at a time. There are several possible approaches. 1 approach would be to get all of the links and use their href values to get an array of id's (['item1', 'item2', 'item3']). This would be the desired approach, as then you could easily add more links and content without having to change your JavaScript code at all. However, for the sake of time, start by just creating an array manually:

var contentBlocks = ['item1', 'item2', 'item3'];

Then attach an event listener or handler to each of the links' onclick event. I prefer event listeners (you can have multiple listeners assigned to a single event, whereas you can only have single event handler assigned to an event), but for the sake of simplicity I'm going to use an event handler. Note, I also don't recommend adding event handlers to your markup... instead, assign them to the elements from the JavaScript, keeping your markup pure. However, again, for the sake of getting a working example quickly, I'm just going to use inline event handlers:

<a href="#item1" onclick="showItem(this.href);">Item 1</a>

Then in your showItem function, we're going to find the id of the item, hide all of the items, and then show only the one that matches.

var contentBlocks = ['item1', 'item2', 'item3'];
function showItem(str) {
  var id = str.substr(str.indexOf('#') + 1),
    el = document.getElementById(id),
    i,
    n;
  for (i = 0, n = contentBlocks.length; i < n; i++) {
    (document.getElementById(contentBlocks[i])).style.display = 'none';
    el.style.display = '';
  }
}

That's basically it (note, I did not test any of this, so make sure it works). There's a lot of room for improvement here, including modifying this example to use unobtrusive JavaScript (removing the inline event handlers), using event listeners instead, and using JavaScript to find the list of id's instead of relying on a hard-coded array. But hopefully this gets you moving in the right direction.

[Fotiman]

Note also that instead of setting display to block, I'm just setting it to an empty string, so it will resort to whatever the default for that element is (could be block, could be table, etc.)

[droverholidays]

Hi Fotiman

Thanks very much for your detailed answer. The problem with your solution (and sorry, I should have mentioned this, but didn't think it was relevant) is that the links are also doing other things - one of which is changing an image displayed on the page in place of a "placeholder". So the href="" is already filled, used to tell the other script which image to load.

I guess innerHTML is a better solution, then? If so, any pointers on what I need to change?

Thanks for the pointer regarding non-JS enabled clients, too. To accommodate these users, I've got a <noscript> section at the top of the page, which explains that JS is needed for full functionality but advises them to scroll down for full details. Then another <noscript> block at the bottom of the page containing all the relevant info but without the fancy layout. Hopefully this is seen as OK?

[Fotiman]

the links are also doing other things - one of which is changing an image displayed on the page in place of a "placeholder". So the href="" is already filled, used to tell the other script which image to load.

Well, you don't have to use the href value for pointing to the content you want to show (though, in my opinion, it would be the preferred method). You could, as I indicated, just have the array hard coded, which would simply mean that you'd need to modify it any time you wanted to add additional content to be toggled in this way. So instead of just an array of id's, you'd need to map something unique about each link to the id of the content block. You could simply add an id to each link for that matter, as in:

<a href="..." id="item1link">Item 1</a>

Then your array might become:

var contentBlocks = [
  ['item1link', 'item1'], 
  ['item2link', 'item2'], 
  ['item3link', 'item3']
]; 

The general concept would be the same. When an item was clicked, you'd pass in this.id, then you would iterate through the contentBlocks array and hide contentBlocks[i][1], and then show contentBlocks[x][1] (where x is the index of contextBlocks who's [0] value matches the id passed in).

Alternatively, you could store them as a hash map of sorts, as in:

var contextBlocks = {
  'item1link': 'item1',
  'item2link': 'item2',
  'item3link': 'item3'
};

And then iterate through this object:

for (o in contentBlocks) {
  if (o == theIdValuePassedIn) {
    (document.getElementById(contentBlocks[o])).style.display = '';
  }
  else {
    (document.getElementById(contentBlocks[o])).style.display = 'none';
  }
}

Something like that.

Regarding the non-JS enabled clients... Sure, you *could* have multiple <noscript> elements on the page, but it just means you need to duplicate your content, which adds extra weight to your page, and means you have more places where discrepancies can creep in. You're only adding more work for yourself by doing that. When you can have a single document that is enhanced for JavaScript enabled clients, you reduce the amount of work needed to maintain that page.

[droverholidays]

Great - thanks again. Will give this a go and report back!