JS - wysuwające się menu

Cześć,

dzisiaj zacząłem się uczyć JS i próbuję sobie teraz zrobić rozwijane menu. W pewnym sensie zrobiłem to, jednak nie rozumiem dlaczego pewien kod, który mi się wydaje, że powinien działać, nie działa. Działający kod osiągnąłem metodą prób i błędów. W kodzie który podam jest bezsensownie wstawiona jedna funkcja w drugiej, proszę nie zwracać na to uwagi, ponieważ później chcę to rozwinąć dlatego tak dałem, jednak nie w tym problem. Kody są wywoływane przy onClick np. onClick="show(1)". Oto kody:

Działający (nie ma ograniczenia do kiedy ma się rozwinąć, ale to nieważne w tym momencie):

	<script>
		var cH = 0;
		function show( n ) {
			unwrap( n );
		}
		function unwrap( n ) {
			cH += 3;
			var div = document.getElementById( "SUB" ).getElementsByTagName( "div" );
			div[n].style.height = cH + "px";
			setTimeout( "unwrap(" + n + ")", 1 );
		}
	</script>

Nie działający (dlaczego on nie działa? Dokładnie to zwiększa mi wysokość o 3 ale tylko raz, tak jakby setTimeout nie chciał się wykonać):

	<script>
		var cH = 0;
		function show( n ) {
			var div = document.getElementById( "SUB" ).getElementsByTagName( "div" );
			unwrap( div[n] );
		}
		function unwrap( div ) {
			cH += 3;
			div.style.height = cH + "px";
			setTimeout( "unwrap(" + div + ")", 1 );
		}
	</script>

Pozdrawiam i dziękuję za pomoc.

Nie działa, bo bezsensownie próbujesz użyć eval(tekstKoduDoWykonania). Nigdy nie używaj tej wersji setTimeout!

Za pierwszy argument podajesz jej tekst kodu, który ma zostać wykonany. Zwykły tekst. Tutaj tekst składasz ze stringów. Do ciągu unwrap( dodajesz [zaraz powiem co], a do tego dodajesz jeszcze ciąg ). I taki tekst każesz wykonać za 1 milisekundę jako skrypt.

Problem jest w tym "[zaraz powiem co]". Do tekstu skryptu do wykonania dodajesz tam wartość zmiennej div. Wartością tej zmiennej jest pewien konkretny element HTML div. Kiedy próbujesz połączyć tę wartość z tekstem za pomocą operatora +, JavaScript próbuje zamienić tę wartość (element DOM) na ciąg znaków. Korzysta przy tym z metody div.toString(). Metoda ta zwraca ciąg [object HTMLDivElement].

Czyli w sumie Twój tekst skryptu do wykonania wygląda tak:

unwrap([object HTMLDivElement]);

To jest bez sensu. Spróbuj napisać takie coś normalnie w pliku ze skryptem. To będzie błąd składni. Właśnie dlatego nie podajemy funkcji setTimeout tekstu skryptu do wykonania: bo ciężko tam znaleźć błąd i nigdy do końca nie wiadomo, jak ten kod skryptu będzie wyglądał w chwili gdy będzie wykonywany.

Zauważ, że gdy "naprawiłeś" ten kod, to przekazujesz tam liczbę -- numer div-a. I tekst skryptu, który przekazujesz tam do setTimeout wygląda o wiele logiczniej. Np. dla diva nr 3:

unwrap(3);

Tylko to też jest generowanie tekstu skryptu do wykonania.

Zamiast takiego wywołania setTimeout zastosuj takie, w którym za pierwszy argument podajesz FUNKCJĘ do wykonania. Nie żaden "tekst funkcji", tylko funkcję.

Czyli nie tak:

setTimeout("nazwa_funkcji()", ...);

Tylko tak:

setTimeout(nazwa_funkcji, ...);

Zauważ, że w drugim przypadku piszemy bez nawiasów: nazwa_funkcji, a nie nazwa_funkcji()! Gdybyś wstawił tam nawiasy, to do setTimeout przekazałbyś nie funkcję nazwa_funkcji, tylko wynik wywołania nazwa_funkcji() (wywołanie nastąpiłoby natychmiast).

Mógłbyś więc napisać setTimeout(unwrap) gdyby tylko unwrap nie miało żadnych argumentów. Bo przekazana w ten sposób funkcja zostanie wywołana bez żadnych argumentów.

Jeśli potrzebujesz argumentów, możesz użyć tzw. domknięcia i funkcji anonimowej. To trochę skomplikowane jak na pierwszy dzień nauki JavaScriptu, ale programiści JS używają tego na co dzień.

Wygląda to tak:

                function unwrap( div ) {
                        cH += 3;
                        div.style.height = cH + "px";
                        setTimeout(function() {
                          unwrap(div);
                        }, 1 );
                }

Do setTimeout przekazujemy funkcję, która nie potrzebuje żadnych parametrów. A co robi? Niewiele. Tylko odpala "kolejne" wywołanieunwrap(), przekazując jej za parametr zmienną div, która pochodzi z "bieżącego" wywołania unwrap.

Ja jeszcze dodam, że nie warto za każdym razem wymyślać koła na nowo. Jeśli uczysz się js, to opanuj jakiś framework (np. jQuery), wtedy stworzenie kodu odpowiedzialnego za animacje menu ogranicza się do jednej linijki kodu.

@sirkruk:
Z drugiej strony, warto się uczyć. Również na praktycznych przykładach. jQuery kilka lat temu nie istniało i ludzie sobie jakoś radzili. Poza tym, jQuery też ktoś musiał napisać. Smutna jest sytuacja, w której niby-programiści JavaScript nie potrafią sobie poradzić bez żadnego frameworka. Wykorzystanie kilkudziesięciu kilobajtów biblioteki do obsługi jednej małej animacyjki byłoby... marnotrawstwem.

Wg mnie warto poznać czy to DOM, czy to posługiwanie się timerami. Nawet jeśli to pierwsze jest niewygodne. Pojawia się to nieraz nawet na rozmowach kwalifikacyjnych dla frontendowców. Np. napisz w JavaScripcie kod, który wybierze wszystkie elementy o selektorze div .foobar p.

Także ćwiczyć sobie zawsze można. A że w prawdziwych projektach warto używać jQuery (czy innej biblioteki) i korzystać z gotowych rozwiązań, to nie ulega wątpliwości.

@jajcek:
Aha: w tym Twoim kodzie masz nieskończoną pętlę. Zwiększasz zmienną cH (i wysokość elementu) w nieskończoność, bez warunku stopu.

Napisałem kod do animacji, jednak nie wiem czy jest on do końca poprawny, ponieważ działa na Chromie, FF i Opera, ale nie działa, wiadomo na czym... na IE 8. A dokładniej: na początku menu się wysunie, ale potem jak chce wysunąć inne menu to stare się chowa (tak jak być powinno), ale nowe się już nie pokazuje tylko IE na dole mówi, że jest błąd na stronie...

	<script>
		var cH = 0;
		var lastItem = 0;
		var speed = 5;
		var go = 1;
		function show( n ) {
			var divCnt = document.getElementById( "SUB" ).getElementsByTagName( "div" ).length;
			var div = document.getElementById( "SUB" ).getElementsByTagName( "div" );
			if( cH != 0 ) {
				wrap( div[lastItem], div[n] );
				go = 0;
			}
			lastItem = n;
			if( go == 1 ) { // tu jest sytuacja początkowa gdy nie ma jeszcze wysuniętego żadnego menu
				unwrap( div[n] );
			}
		}
		function wrap( div, divn ) {
			cH -= speed;
			div.style.height = cH + "px";
			if( cH >= 0 ) {
				setTimeout( function() {
					wrap( div, divn );
				}, 1 );
			} else {
				cH = 0;
				unwrap( divn );
			}
		}
		function unwrap( div ) {
			cH += speed;
			div.style.height = cH + "px";
			if( cH <= div.getElementsByTagName( "a" ).length * 20 ) {
				setTimeout( function() {
					unwrap( div );
				}, 1 );
			} else cH = div.getElementsByTagName( "a" ).length * 20;
		}
	</script>

@jajcek:
Zamieść przykładowy, pełny dokument HTML wraz z kodem menu. Wywal tyle rzeczy ile się da, ale zostaw wszystko co potrzebne. Tak, bym mógł sobie przekleić do edytora, zapisać i odpalić w IE8. Ewentualnie daj link do (nie)działającej strony.

Lub sprawdź to samodzielnie tak, jak ja to zrobię: zobacz, o jakim błędzie mówi IE. Jak niczego konkretnego nie mówi, to użyj debuggera lub nawet sprawdź bamberskimi alertami(), metodą połowienia, gdzie jest błąd.

BTW. nie pisz nigdy takich powtarzających się fragmentów kodu:

var divCnt = document.getElementById( "SUB" ).getElementsByTagName( "div" ).length;
var div = document.getElementById( "SUB" ).getElementsByTagName( "div" );

Zasada DRY (Don't Repeat Yourself) jest w top 3 czy top 2 zasad pisania przyzwoitego kodu. Tutaj, powtarzasz dwa razy document.getElementById( "SUB" ).getElementsByTagName( "div" ). Razi to tym bardziej, że wynik tego zapytania do DOM przechowujesz i tak w zmiennej div.

Zamiast powyższego kodu, napisz:

var divs = document.getElementById( "SUB" ).getElementsByTagName( "div" );
var divCnt = divs.length;

Zauważ, że zmieniłem nazwę z div na divs, bo w rzeczywistości zmienna ta przechowuje nie jednego diva, tylko kilka.

Podobne powtórzenie masz w funkcji unwrap, gdzie powtarzasz div.getElementsByTagName( "a" ).length * 20. Zapisz całe to wyrażenie w jakiejś zmiennej z opisową nazwą. Np. maxHeight. Zobaczysz, że kod stanie się bardziej przejrzysty:

                function unwrap( div ) {
                        cH += speed;
                        div.style.height = cH + "px";
                        var maxHeight = div.getElementsByTagName( "a" ).length * 20;
                        if (cH <= maxHeight ) {
                                setTimeout( function() {
                                        unwrap( div );
                                }, 1 );
                        } else {
                                cH = maxHeight;
                        }
                }

Poprawiłem to o czym wspomniałeś. Dziękuję za uwagi. Tutaj jest pełny kod:

<!DOCTYPE html>
<html>
<head>
	<script>
		var cH = 0;
		var lastItem = 0;
		var speed = 8;
		var go = 1;
		function show( n ) {
			var divs = document.getElementById( "SUB" ).getElementsByTagName( "div" );
			
			if( cH != 0 ) {
				wrap( divs[lastItem], divs[n] );
				go = 0;
			}
			lastItem = n;
			if( go == 1 ) {
				unwrap( divs[n] );
			}
		}
		function wrap( div, divn ) {
			cH -= speed;
			div.style.height = cH + "px";
			if( cH >= 0 ) {
				setTimeout( function() {
					wrap( div, divn );
				}, 1 );
			} else {
				cH = 0;
				div.style.height = cH + "px";
				unwrap( divn );
			}
		}
		function unwrap( div ) {
			cH += speed;
			div.style.height = cH + "px";
			var maxH = div.getElementsByTagName( "a" ).length * 20;
			if( cH <= maxH ) {
				setTimeout( function() {
					unwrap( div );
				}, 1 );
			} else {
				cH = maxH;
				div.style.height = cH + "px";
			}
		}
	</script>
	<style>
		#SUB div {
			height: 0px;
			width: 167px;
			overflow: hidden;
		}
	</style>
</head>
<body>
	<a href="#" onClick="show( 0 )">link do wysuniecia menu</a><br>
	
	<div id="SUB">
		<div>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a><br>
			<a href="#">link1</a>
		</div>
	</div>
</body>
</html>

 function wrap( div, divn ) {
                        cH -= speed;
                        div.style.height = cH + "px"; 

Błąd występuje w tym fragmencie. cH jest ujemny w jednej z iteracji, i nie można ustawić ujemnej wartości height.

rzeczywiście zadziałało, już myślałem, że IE jak zwykle głupieje, a to jednak moja wina ;)

dziękuję