Javascript: Timed Banner Not Working

[Vimeous]

After yet more trawling for decent code and a healthly dislike for easy-way-out prebuilt code packs I've run into a javascript issue I've so far failed to leaver myself out of.

Can you see where I've gone wrong?

Aim: A timed banner which consists of multiple hidden divs'.
Code:

var c=0;
var t;

function timedBanner(divs)
{
switchBanner(c);
c=c+1;
if (c > divs -1) {
c = 0;
}
t=setTimeout("timedBanner()",1000);
}

function switchBanner(obj) {

var banners = new Array();
banners[0] = document.getElementById("serviceBanner1").innerHTML;
banners[1] = document.getElementById("serviceBanner2").innerHTML;

document.getElementById("mainBanner").innerHTML = banners[obj];
}

timedBanner is called from the body onload and currently set to timedBanner(2)
mainBanner is a div containing 2 hidden divs called serviceBanner1 and serviceBanner2

Problem: The initial banner loads correctly. On the second pass I get an objectexpected error on the line starting banners[1]


Why?

No doubt it's a small problem but I can't see it at present. Help please!

[scaryjim]

function timedBanner(divs)
{
    switchBanner(c);
    c=c+1;
    if (c > divs -1) {
        c = 0;
    }

    t=setTimeout("timedBanner()",1000);
}

Your issue is the line in bold: function timedBanner() expects a parameter! This should be:

t=setTimeout("timedBanner(divs)",1000);

[Vimeous]

function timedBanner(divs)
{
    switchBanner(c);
    c=c+1;
    if (c > divs -1) {
        c = 0;
    }

    t=setTimeout("timedBanner()",1000);
}

Your issue is the line in bold: function timedBanner() expects a parameter! This should be:

t=setTimeout("timedBanner(divs)",1000);

Thanks Jim. I'd taken it out thinking it'd stop the loop running ad-infinitum but I see that would not be the case (and I was getting a different error).

I now get an error that with "divs" being undefined.

How is it best to define "divs" in this circumstance?

[scaryjim]

Oops! sorry, I forgot that setTimeout takes a string for the command to run. You'll need to do

t=setTimeout("timedBanner(" + divs + ")",1000);

to put the value of divs into the string. Apparently being a professional web app developer doesn't make you immune from stupid mistakes (who'd'a thunk it, huh? )!

EDIT: it doesn't make me immune to writing questionable / deprecated code, either! There's two versions of setTimeout, the one you're using which runs some text input as a code snippet, and a newer version that takes a function object (I won't go into the complexity of *everything* being an object in javascript) that you can then add parameters to. The generalised form is: window.setTimeout(func, delay, [param1, param2, ...]);, so your code snippet should be

t=setTimeout(timedBanner, 1000, divs);

[Vimeous]

Oops! sorry, I forgot that setTimeout takes a string for the command to run. You'll need to do

t=setTimeout("timedBanner(" + divs + ")",1000);

to put the value of divs into the string. Apparently being a professional web app developer doesn't make you immune from stupid mistakes (who'd'a thunk it, huh? )!

EDIT: it doesn't make me immune to writing questionable / deprecated code, either! There's two versions of setTimeout, the one you're using which runs some text input as a code snippet, and a newer version that takes a function object (I won't go into the complexity of *everything* being an object in javascript) that you can then add parameters to. The generalised form is: window.setTimeout(func, delay, [param1, param2, ...]);, so your code snippet should be

t=setTimeout(timedBanner, 1000, divs);

A definite improvement! ...... but now I'm back to the previous error:

In IE7/8 I get an objectexpected error on the line starting banners[1].
In Firefox 3.5 I get document.getElementById("serviceBanner1") is null.

Both errors imply I've got the function switchBanner wrong. The HTML code for divs themselves is:

<div id="mainBanner">

<div id="serviceBanner1" class="hide">
<img src="image1.jpg" class="" alt="x" title="x" width="835" height="350" border="0" usemap="#xMap1" />
<map name="xMap1" id="xMap1">
<area shape="rect" coords="460,70,615,120" href="x.html" title="x" alt="x" class="cursor" />
</map>
</div>

<div id="serviceBanner2" class="hide">
<img src="image2.jpg" class="" alt="y" title="y" width="835" height="350" border="0" usemap="#yMap1" />
<map name="yMap1" id="yMap1">
<area shape="rect" coords="485,175,645,220" href="y.html" title="y" alt="y" class="cursor" />
</map>
</div>

</div>

I wonder if it's related to the divs being hidden.
That said both browsers display the first banner and only trigger an error when the running the subsequent code.

Confused!


n.b.
Sorry if you cringed at the lack of class for the images - that'll appear if it starts working

[scaryjim]

A definite improvement! ...... but now I'm back to the previous error:

In IE7/8 I get an objectexpected error on the line starting banners[1].
In Firefox 3.5 I get document.getElementById("serviceBanner1") is null.

Both errors imply I've got the function switchBanner wrong.

<snip>

Hmm, you have indeed got the code for switchBanner wrong! The first time the code is called, it rewrites over the whole of the content of mainBanner with the content of serviceBanner1. This [b]writes over[/i] the content of both serviceBanner1 and servicebanner2, so they no longer exist in the document tree. Any future attempt to reference either of them is doomed to failure, because they don't exist anymore!

Sorry, I didn't even look at your switchBanner code before because I'd spotted a problem in your earlier code (and frankly I wasn't treating this like a formal code review ).

if you want to do it this way I *think* setting up the banners array outside of the function might work.

A better way to change whether you can see a DIV programatically is to set its .style.display property:

document.getElementById("serviceBanner1").style.display = "block";
document.getElementById("serviceBanner2").style.display = "none";

will turn off display of serviceBanner2 and show serviceBanner1 instead. While you've only got 2 or 3 divs to roll through this works OK: more than that and you need to look at enumerating the children of a parent (in this case your "mainBanner" div). But for now I'd play with using your c variable with an if statement and doing it manually, just to get it working: there's a few complexities and pitfalls to watch out for when enumerating the children of an element...

Sorry if you cringed at the lack of class for the images - that'll appear if it starts working

I'm not concerned by the lack of a class so much as putting the attribute in when you're not using it but as you say, that'll come with time...

[Vimeous]

Hmm, you have indeed got the code for switchBanner wrong! The first time the code is called, it rewrites over the whole of the content of mainBanner with the content of serviceBanner1. This [b]writes over[/i] the content of both serviceBanner1 and servicebanner2, so they no longer exist in the document tree. Any future attempt to reference either of them is doomed to failure, because they don't exist anymore!

Sorry, I didn't even look at your switchBanner code before because I'd spotted a problem in your earlier code (and frankly I wasn't treating this like a formal code review ).

if you want to do it this way I *think* setting up the banners array outside of the function might work.

A better way to change whether you can see a DIV programatically is to set its .style.display property:

document.getElementById("serviceBanner1").style.display = "block";
document.getElementById("serviceBanner2").style.display = "none";

will turn off display of serviceBanner2 and show serviceBanner1 instead. While you've only got 2 or 3 divs to roll through this works OK: more than that and you need to look at enumerating the children of a parent (in this case your "mainBanner" div). But for now I'd play with using your c variable with an if statement and doing it manually, just to get it working: there's a few complexities and pitfalls to watch out for when enumerating the children of an element...

I'm not concerned by the lack of a class so much as putting the attribute in when you're not using it but as you say, that'll come with time...

BIG thanks for pointing out I was overwriting the code. I'm definitely not a pro-web-app-dev!

That said I reckon I can get an alternate solution working to those you suggest. Instead of replacing the content of mainBanner I just need to add an empty named div as the content of mainBanner is hidden. Then I can replace the content of that as often as I like

Of course I've just tried this and I seem to have too many entires ...... but it is has shown divs 1 and 2 this time lol
(actually it works in firefox but ie is declaring issues with undefined stuff hmm.....)

[Vimeous]

t=setTimeout("timedBanner(" + divs + ")",1000);

t=setTimeout(timedBanner, 1000, divs);

scary I've double checked the code again and it turns out only the longer original code you suggested (top one) will work with both Firefox 3.5 and IE 7. I'll check IE 8 shortly.

Many thanks for your help.
While I know I've 'cheated' by using another empty div it also keeps the code simpler for myself and those who follow to maintain.

Now I'm off to canabalise the fade in/out I discovered recently

[scaryjim]

scary I've double checked the code again and it turns out only the longer original code you suggested (top one) will work with both Firefox 3.5 and IE 7.

Hmm, quite possible. IE prior to 8 didn't bother about following the standards too closely! if there's a version that works with everything stick with that one

You should find the second version works OK in IE8 I think - in my experience so far they've done a much better job of making it adhere to the standard... my biggest problem is that some of the sites I design for a living are targetted at the NHS and some NHS trusts still haven't upgraded from IE6 because their in-house developed web apps are coded for specific IE6 hacks, so not only do I have to code for several different browsers, I also have to be prepared for 3 different versions of IE!

[yamangman]

Or you could just use jquery and not spend 4 days completeing the simplest of scripts