Bilderpopup

[misterunknown]

Moin,
ich habe mir vor einiger Zeit ein eigenes kleines Skript geschieben, mit dem man Bilder groß anzeigen kann. Sie legen sich über die Seite.
Dieses Skript ist hier zu finden:
http://misterunknown.de/site/js/piclayer.js

Nachdem es dann irgendwann funktioniert hat, habe ich das Chaos an Funktionen und Befehlen etwas entwirrt und geordnet und unnötiges entfernt. Was sagt ihr dazu? Kann ich einige Sachen besser machen? Was ist schlechter Stil oder lässt sich anders einfacher lösen?

Wer mal Muße hat, kann es sich ja mal angucken.

Grüße Marco

[suit]

Wer mal Muße hat, kann es sich ja mal angucken.

Spontan fehlten mir diverse Dinge wie etwa "Index des aktuellen Bildes" lesen und "Index auf ein beliebiges Bild setzen" - nur vor und zurück ist etwas mager.

Übrigens: Schei? Encoding :p

[misterunknown]

Moin,

Spontan fehlten mir diverse Dinge wie etwa "Index des aktuellen Bildes" lesen und "Index auf ein beliebiges Bild setzen" - nur vor und zurück ist etwas mager.

Hm, könnte man noch hinzufügen, allerdings wozu? Wenn man eine Galerie hat, und ein bestimmtes Bild haben will, macht man halt das große klein, und klickt auf das gewollte... Oder?

Übrigens: Schei? Encoding :p

Ja, es ist kein UTF-8^^

Grüße Marco

[Steel]

Moin!

Hm, könnte man noch hinzufügen, allerdings wozu? Wenn man eine Galerie hat, und ein bestimmtes Bild haben will, macht man halt das große klein, und klickt auf das gewollte... Oder?

Ich schon. Aber nach mir darf man sich bei sowas scheinbar nicht richten.

[suit]

Hm, könnte man noch hinzufügen, allerdings wozu?

Du hast nicht vor, dass jemand anderer dein Script verwendet?

Wenn man eine Galerie hat, und ein bestimmtes Bild haben will, macht man halt das große klein, und klickt auf das gewollte... Oder?

Wenn du z.B. einen Thumbstrip dazubaust, musst du gezielt Bilder wählen können.

Da klickst du dann das dritte Thumbnail und willst das angezeigt haben.

Ich kann dir grade kein fertiges Beispiel geben (NDA, eh klar) aber kürzlich hab' ich sowas für einen Kunden gebaut.

Meine Basis für die Bildvergrößerung ist hierbei Colorbox - da aber eben entsprechende Methoden fehlen, musste ich das dazubauen:

https://github.com/jackmoore/colorbox/issues/55

[misterunknown]

Moin,

Du hast nicht vor, dass jemand anderer dein Script verwendet?

Also ich sag mal so: Wenn man ein Skript mit allem drum und dran will, würde selbst ich Highslide nehmen. Grundsätzlich bin ich bestrebt das Skript so einfach wie möglich zu halten, so dass ich es in einem halben Jahr immer noch verstehe ;)

Wenn du z.B. einen Thumbstrip dazubaust, musst du gezielt Bilder wählen können.

Hm, ja das stimmt. Allerdings könnte man auch einfach den Bildern ins onclick-Attribut schreiben, dass es das Bild anzeigen lassen soll. Die Funktion showPic() kann ja einzelne Bilder direkt aufmachen. Nur ein auslesen des Indexes ist noch nicht vorgesehen.

Colorbox kannte ich gar nicht. Sieht aber auch ganz gut aus. Ich habe mal versucht mit einem Fading-Effekt zu arbeiten, aber habs nicht wirklich hinbekommen. Ich hatte auch nicht viel Zeit dafür.

Grüße Marco

[suit]

Also ich sag mal so: Wenn man ein Skript mit allem drum und dran will, würde selbst ich Highslide nehmen.

Highslide ist überladener Mist :)

Grundsätzlich bin ich bestrebt das Skript so einfach wie möglich zu halten, so dass ich es in einem halben Jahr immer noch verstehe ;)

Das ist richtig - aber ein paar Schnittstellen zum Abfragen und setzen der Parameter müssen schon vorhanden sein.

Eine Konfigurierbarkeit wie bei Highslide hingegen ist absolut unnötig - da werden dutzende Skins mitgeliefert die man irgendwie zusammenkonfigurieren kann - das absolut unsinnig.

Das Ding soll die nötigen DOM-Manipulationen durchführen und ein paar Schnittstellen bereitstellen, das Styling macht man dann über CSS.

[Kai345]

[latex]Mae  govannen![/latex]

http://misterunknown.de/site/js/piclayer.js

Nachdem es dann irgendwann funktioniert hat, habe ich das Chaos an Funktionen und Befehlen etwas entwirrt und geordnet und unnötiges entfernt. Was sagt ihr dazu? Kann ich einige Sachen besser machen? Was ist schlechter Stil oder lässt sich anders einfacher lösen?

Ein paar Kleinigkeiten fallen mir spontan auf:

Du hast _entschieden_ zu viele document.getElementById("piclayer") im Code. Besser ist es, die Referenz auf dieses Objekt nur _ein_ Mal zu ermitteln und in einer Variablen abzulegen und danach nur noch mit dieser Referenz zu arbeiten.

Dann noch in "keyHandler":

1: if(!evv)evv = window.event;  
2:    if(document.getElementById("piclayer").style.display == "block"){  
3:        evv.preventDefault();  
4:        if(evv.which)taste = evv.which;  
5:        else taste = evv.keyCode;

In Zeile 3  fehlt, falls noch benötigt, die Alternative für alte IE zu evv.preventDefault():

if (evv.preventDefault)  
    evv.preventDefault()  
else  
    evv.returnValue = false;

die Zeilen 4 + 5 sind verwirrend formatiert, "taste" ist eine globale Variable, außerdem würde ich hier das if-else-Konstrukt entfernen :

var taste = evv.which || evv.keyCode;

oder

var taste = (evv.which) ? evv.which : evv.keyCode;

Stur lächeln und winken, Männer!
Kai

[misterunknown]

Moin,

Ein paar Kleinigkeiten fallen mir spontan auf:
Du hast _entschieden_ zu viele document.getElementById("piclayer") im Code. Besser ist es, die Referenz auf dieses Objekt nur _ein_ Mal zu ermitteln und in einer Variablen abzulegen und danach nur noch mit dieser Referenz zu arbeiten.

Ich habe es jetzt in jeder Funktion einmal. Oder würde das auch funktionieren, wenn ich in der init-Funktion eine globale Variable anlege?

Dann ist mir noch folgendes unklar: in der Funktion showPic() teste ich, ob das Objekt vorhanden ist. Kurz darauf weise ich einer Variablen das Objekt zu:

if(!document.getElementById("piclayer"))mdl.init();  
[...]  
piclayer = document.getElementById('piclayer');

Kann ich das noch irgendwie zusammenfassen?
Folgendes funktioniert nicht:
if(!var piclayer = document.getElementById('piclayer');

In Zeile 3  fehlt, falls noch benötigt, die Alternative für alte IE zu evv.preventDefault():

Das ist gut, mit dem IE hatte ich das noch gar nicht probiert.

var taste = evv.which || evv.keyCode;

Das habe ich auch so geändert, sieht viel übersichtlicher aus.

Ich danke dir für deine Tipps.

Grüße Marco

[Der Martin]

Hallo,

Ein paar Kleinigkeiten fallen mir spontan auf:
Du hast _entschieden_ zu viele document.getElementById("piclayer") im Code. Besser ist es, die Referenz auf dieses Objekt nur _ein_ Mal zu ermitteln und in einer Variablen abzulegen und danach nur noch mit dieser Referenz zu arbeiten.
Ich habe es jetzt in jeder Funktion einmal. Oder würde das auch funktionieren, wenn ich in der init-Funktion eine globale Variable anlege?

ja, und das würde ich in so einem Fall vielleicht sogar tun, auch wenn man globale Variablen sonst eigentlich vermeiden möchte.

if(!document.getElementById("piclayer"))mdl.init();

[...]
piclayer = document.getElementById('piclayer');

> Kann ich das noch irgendwie zusammenfassen?  
  
Klar, aber nicht so, wie du es versucht hast.  
  

> Folgendes funktioniert nicht:  
> `if(!var piclayer = document.getElementById('piclayer');`{:.language-javascript}  
  
Stimmt. Eine Deklaration (Keyword var) in der if-Klammer ist nicht erlaubt. Zieh die Deklaration vor:  
  

> ~~~javascript

var piclayer;  

> if (!(piclayer=document.getElementById('piclayer'))) ...

Das Semikolon nach der if-Klammer kann dir übrigens viele Stunden spannender Fehlersuche bescheren. Und zähl mal die Klammern in deinem Beispiel. ;-)

Übersichtlicher sieht das außerdem aus, wenn man die Bedingung in der if-Klammer nicht negiert und stattdessen den if- und den else-Anweisungsblock vertauscht - vorausgesetzt, man hat einen else-Block.

Ciao,
 Martin

[Kai345]

[latex]Mae  govannen![/latex]

Du hast _entschieden_ zu viele document.getElementById("piclayer") im Code. Besser ist es, die Referenz auf dieses Objekt nur _ein_ Mal zu ermitteln und in einer Variablen abzulegen und danach nur noch mit dieser Referenz zu arbeiten.
Ich habe es jetzt in jeder Funktion einmal. Oder würde das auch funktionieren, wenn ich in der init-Funktion eine globale Variable anlege?

Auf keinen Fall. Dein Script sollte maximal eine neue Variable anlegen: picLayer. Hänge die Referenz einfach als Eigenschaft an dein Objekt.

var picLayer = {  
        picRef:  
            null,  
        init:  
            function () {  
                var mdlimg = document.getElementById("piclayer");  
                if(!mdlimg){  
                mdlimg = document.createElement("img");  
                mdlimg.id = "piclayer";  
                mdlimg.style.position = "absolute";  
                mdlimg.style.display = "none";  
                mdlimg.onclick = function () { picLayer.picRef.style.display = "none"; };  
                document.body.appendChild(mdlimg);  
                }  
                picLayer.picRef = mdlimg;  
            },  
[...]  

(ungetestet, nur zur Verdeutlichung)

Entsprechend dann diese Referenz im Code überall verwenden.

Was soll eigentlich do{test = "test";}while(loadPic.onload); bedeuten/-zwecken?

BTW: Du hast da noch ein paar globale Variablen im Code, entweder auch ans Objekt hängen oder mit var deklarieren, je nach Verwendungszweck

Stur lächeln und winken, Männer!
Kai