galeria z oknem modalnym (JS)

Tak na próbę i z ciekawości przedstawię kod JS, który obsługuje galerię z oknami modalnymi. Oczywiście zdaję sobie sprawę, że tutaj za dużo do oceny nie ma w porównaniu z innymi projektami. Ale być może coś wartego poprawki w moim stylu wypatrzycie...

let modal_okno = document.getElementById("modal_okno");
let modal_obraz = document.getElementById("modal_obraz");
let obrazki = document.getElementsByClassName("mini");
let indeks;

// klikniÄcie dowolnego obrazka wyĹwietla okno modal
for (let i = 0 ; i < obrazki.length ; i++)
{
    obrazki[i].onclick = function()
    {
        modal_okno.style.display = "block";
        indeks = this.id;
        pokaz_obraz();
    }
}

// przyisk X (zamknij)
document.getElementById("modal_zamknij").onclick = function() 
{
    modal_okno.style.display = "none";
}

// przycisk PREV
document.getElementById("modal_prev").onclick = function() 
{  
    lewo_prev();
} 

// przycik NEXT
document.getElementById("modal_next").onclick = function() 
{
    prawo_next();
}

// obsĹuga strzaĹek lewa/prawa z klawiatury
document.onkeydown = function(e)
{
    if (modal_okno.style.display == "block")
    {
        // lewa
        if (e.keyCode == 37)
        {
            lewo_prev();
        }
        // prawa
        if (e.keyCode == 39)
        {
           prawo_next();
        }
    }
} 

function pokaz_obraz()
{
    modal_obraz.src = "(" + indeks + ").jpg";
    document.getElementById("modal_numer").textContent = indeks;
}

function lewo_prev()
{
    if (indeks > 1) {indeks--;}
    pokaz_obraz();
}

function prawo_next()
{
    if (indeks < obrazki.length) {indeks++;}
    pokaz_obraz();
}

document.getElementById("modal_prev").onclick = function()
{

subiektywna kwestia, ale w JS styl "klamerka w nowej linii" raczej jest rzadko spotykany.
Bardziej popularny jest taki sposób pisania:

 function() {

można to też skrócić i użyć arrow function:

document.getElementById("modal_zamknij").onclick = (e) => {
  //......
};

tylko wtedy nie masz this, ale nie szkodzi (w sensie i tak masz e.target)

Poza tym masz część zmiennych po polsku. Co owocuje tego typu nazewnictwem:

"modal_zamknij"
"modal_prev"
"lewo_prev"
"prawo_next"

już pomijając, że "wypada" pisać po angielsku, to tu jest też problem ze spójnością. Masz modal_zamknij, czyli ANG + POL.
Ale zaraz masz modal_prev, czyli ANG + ANG itp.
Nawet jak nikt tego nie będzie czytał potem, to sam będziesz pewnie miał problemy - czy to było modal_zamknij czy modal_close? Czy to było prawo_next czy right_nastepny? itp. itd. Dlatego dla własnego dobra lepiej wszystko pisać po angielsku.

 pokaz_obraz();

w JS się przyjęła notacja camelCase. Co jest subiektywne, wiadomo, ale tak większość robi. Czyli bardziej pokazObraz(), a jeszcze lepiej showImage().

// klikniÄcie dowolnego obrazka wyĹwietla okno modal

Nie wiem, dlaczego są te krzaczki, ale upewnij się, że zapisujesz plik w odpowiednim kodowaniu (UTF-8), bo takie krzaczki potem mogą ci wyskakiwać, jak będziesz wyświetlał komunikaty użytkownikowi.

 if (indeks > 1) {indeks--;}

w JS indeksy zaczynają się od zera, więc raczej indeks > 0

(indeks < obrazki.length) {indeks++;}

no i kończą się liczbie o jeden mniejszej od length, więc powinno być indeks < obrazki.length - 1

Niestety do poprawki jest cała koncepcja :-)
Główne wady Twojego rozwiązania to:

1. Nie możesz na jednej stronie utworzyć dwóch niezależnych galerii.
2. obiekt "modal_okno" musi istnieć nawet jeśli użytkownik nie będzie chciał powiększać miniaturek.
3. Sposób zastosowania zmiennej indeks jest karygodny!
4. Używasz konstrukcji: obrazki[i].onclick = ... lepiej skorzystać z obrazki[i].addEventListener ( 'click'm function() .. Twoja metoda nadpisuje ewentualną istniejącą już funkcję obsługi tego zdarzenia.
5. Całą pętlę inicjującą przerobiłbym na funkcję, do której przekazuje się jedynie nazwę klasy.
6. Okno z powiększonymi zdjęciami powinno tworzyć się dynamicznie a po zamknięciu galerii ma zostać usunięte z drzewa DOM ( https://developer.mozilla.org/pl/docs/Web/API/Document/createElement )

Nawet zakładając, że nie umiesz jeszcze pisać obiektowo to dało się zrobić wszystko jako zgrabne uniwersalne funkcje, które dają większą elastyczność wykorzystania mechanizmu.

1. Mogłeś dać jakąś wersję działającą, którą dałoby się przeklikać
2. Lepiej jest stosować nazwy angielskie
3. A jak już używasz polskich, to bądź konsekwentny i nie dawaj nazw w stylu lewo_prev
4. document.getElementById("modal_okno"); - czy nie lepiej jest, zamiast wpisywać nazwę klasy na sztywno, to wykorzystać do tego jakąś zmienną? Potem, jeśli będziesz chciał zmienić nazwę tej klasy, to musisz wszędzie to poprawić. A znając życie - gdzieś ominiesz jedno odwołanie i pojawią się trudne do wykrycia problemy
5. zacząłem pisać odpowiedź, a potem musiałem odejść od kompa i widzę, zę wiele rzeczy napisali już chłopacy przede mną :(
6. nie wiem, czy nazywanie obrazków "index"+.jpg jest optymalne. Chociażby ze względu na SEO. Jakbyś nazwał te obrazki kuchnia_domowa_1.jpg to są szanse, że Google to zindeksuje i może ktoś trafi na stronę poprzez wyszukiwarkę

Postaram się odnieść do każdej uwagi, bo są cenne. Dziękuję.

Tak na szybko, dlaczego "Sposób zastosowania zmiennej indeks jest karygodny!"? Chodzi o miejsce deklaracji?

subiektywna kwestia, ale w JS styl "klamerka w nowej linii" raczej jest rzadko spotykany.

Wiem o tym. Dla mnie po prostu mój styl jest bardziej czytelny. Być może to nawyk z innych języków.

w JS indeksy zaczynają się od zera, więc raczej indeks > 0
no i kończą się liczbie o jeden mniejszej od length, więc powinno być indeks < obrazki.length - 1

Takie podstawy to ogarniam, spokojnie :) To wynika ze sposobu nazewnictwa. Nie mam przykładowo obrazka o id = 0

Nie możesz na jednej stronie utworzyć dwóch niezależnych galerii.

Fakt, czytałem pewne "rady" tj. pisz kod tak, by miał postać jak uniwersalnej funkcji.

obiekt "modal_okno" musi istnieć nawet jeśli użytkownik nie będzie chciał powiększać miniaturek.

Okno z powiększonymi zdjęciami powinno tworzyć się dynamicznie a po zamknięciu galerii ma zostać usunięte z drzewa DOM ( https://developer.mozilla.org[...]eb/API/Document/createElement )

I tu jest sprawa, która mnie ciekawi. Mianowicie co jest lepsze i dlaczego?
a) tworzymy/usuwamy element wedle potrzeby
b) pokazujemy/chowamy element wedle potrzeby

Używasz konstrukcji: obrazki[i].onclick = ... lepiej skorzystać z obrazki[i].addEventListener ( 'click'm function() .. Twoja metoda nadpisuje ewentualną >istniejącą już funkcję obsługi tego zdarzenia.

Wiem o tym. Jeżeli jednak oprócz kliknięcia na obrazek nic więcej nie przewiduję, to w czym problem? Bo fakt, jak dodam Listener, to będę miał zapas na przyszłość. Więc to chyba nie błąd jeżeli jeszcze raz podkreślam rozumiem różnicę i wiem co masz na myśli pisząc o nadpisaniu.

document.getElementById("modal_okno"); - czy nie lepiej jest, zamiast wpisywać nazwę klasy na sztywno, to wykorzystać do tego jakąś zmienną? Potem, >jeśli będziesz chciał zmienić nazwę tej klasy, to musisz wszędzie to poprawić. A znając życie - gdzieś ominiesz jedno odwołanie i pojawią się trudne do >wykrycia problemy

Jeżeli dany element używam raz, to po co tworzyć zmienną? Co innego jak dany element modyfikuję w kilku miejscach kodu - tutaj faktycznie lepiej dać zmienną.

nie wiem, czy nazywanie obrazków "index"+.jpg jest optymalne. Chociażby ze względu na SEO. Jakbyś nazwał te obrazki kuchnia_domowa_1.jpg to są >szanse, że Google to zindeksuje i może ktoś trafi na stronę poprzez wyszukiwarkę

Tak je nazwałem na potrzeby testów. A pozycjonowanie nie jest mi potrzebne. Zbyt niszowy projekt.

Jeżeli chodzi o nazewnictwo to racja, bez sensu. Po prostu bardziej skupiłem się na mechanice. A przecież od razu mogłem dopilnować nazw.

kosmonauta80 napisał(a):

subiektywna kwestia, ale w JS styl "klamerka w nowej linii" raczej jest rzadko spotykany.

Wiem o tym. Dla mnie po prostu mój styl jest bardziej czytelny. Być może to nawyk z innych języków.

W porządnych językach możesz sobie wybierać według gustu. W JS musisz mieć w tej samej linii, no chyba, że będziesz robił specjalny wyjątek w swoim podejściu dla return, ale taki niespójny miks będzie wyglądał gorzej niż cokolwiek.