Do sprawdzenia kod JavaScript.

Question

Witam,

Dopiero się uczę JS i bardzo bym prosił o wskazówki dotyczące mojego kodu. Używam czystego JS bez jQuery. 

Kod powoduje pojawienie się animacji po przewinięciu pierwszej sekcji w dół o 100vh. Dodaje klasy i usuwa z poszczególnych tagów. Kod działa poprawnie.

Czy można było lepiej go napisać? Na co zwracać uwagę.

 

 

// ........................ scroll run animation ...................
window.onscroll = function() {runAnimationRun()};
function runAnimationRun(){
    var h = Math.max(document.documentElement.clientHeight, window.innerHeight || 0);
    if (document.body.scrollTop > h || document.documentElement.scrollTop > h) {
        addShadows();
    }
}

// ......................... pointers and shadows...................
function addShadows() {
    document.getElementById('pointer1').classList.add('pointer-down1');
    document.getElementById('pointerShadow1').classList.add('pointer-down1');
    document.getElementById('pointer2').classList.add('pointer-down2');
    document.getElementById('pointerShadow2').classList.add('pointer-down2');
    document.getElementById('pointer3').classList.add('pointer-down3');
    document.getElementById('pointerShadow3').classList.add('pointer-down3');
    document.getElementById('pointer4').classList.add('pointer-down4');
    document.getElementById('pointerShadow4').classList.add('pointer-down4');
    document.getElementById('pointer5').classList.add('pointer-down5');
    document.getElementById('pointerShadow5').classList.add('pointer-down5');
    var shadows = document.getElementsByClassName('fade-in');
    for(var i=0; i < shadows.length; i++) {
        shadows[i].classList.add('shadow-on');
    }
}
// ...................... Speech bubbles ..................
function showSpeechBubbles() {
    let arrWindows= document.getElementsByClassName("windows");
    var timeout = 4000;
    for (let i=0; i<arrWindows.length;  i++) {
            setTimeout( function windowin(){
                arrWindows[i].className += " showing";
            }, timeout);
            var timeout = timeout+4000;
            setTimeout( function windowout(){
                arrWindows[i].classList.remove("showing");
            }, timeout);
    }
}
showSpeechBubbles();
setInterval("showSpeechBubbles()", 20000);

 

Comments

Używam czystego JS bez jQuery. 

---

Nawet nie wiesz jak trudno znaleźć podpowiedzi i rozwiązania problemów w czystym JS. W większości poradników pakują jQuery. Nawet w prostych rozwiązaniach.

---

To dlaczego nie używasz jQuery? Pisząc w nim, kod jest dużo bardziej przejrzysty, a jQuery dużo nie waży.

---

jQuery dużo nie waży.

No tak, w końcu Angular waży 800KB, to takie jQuery – cóż to jest…

 To dlaczego nie używasz jQuery?

A to jakiś przymus? Kod z jQuery wcale nie musi być czytelniejszy. Poza tym warto wiedzieć jak się to robi normalnie, bo jQuery tak dobrze obudowuje DOM, że można go używać nie rozumiejąc co to DOM. 

Answer 1

Super, że używasz JS.  A jeśli szukasz rozwiązań np.  Na stackoverflow to dopisz pure JS. Taka mała rada. Co do kodu,  to zawsze można coś napisać lepiej.

Np.  Zamiast window.onscroll dasz addEventListener. A funkcja addShadows może przyjąć argumenty,  id elementu,  na którym pracujesz i klasę,  którą nadajesz. Wtedy taką funkcję możesz wywołać na tablicy elementów,  czy czymś podobnym.  Masz wtedy przenośną funkcję skoro używasz classlist,  to rób to wszędzie.  Konsekwencją przede wszystkim.  Nad resztą wypowie się pewnie ktoś z większym doświadczeniem.

Comments

Dzięki za rady.

Answer 2

window.onscroll = function() {runAnimationRun()};

Jeśli nie przekazujesz argumentów, po prostu:

window.onscroll = runAnimationRun();

! .on* już się nie używa. !

---

function runAnimationRun(){
    var h = Math.max(document.documentElement.clientHeight, window.innerHeight || 0);

Prawdopodobnie nie musisz liczyć wysokości okna przy każdym scorllu - wystarczy policzyć ją raz.

---

function addShadows() {
    document.getElementById('pointer1').classList.add('pointer-down1');
    document.getElementById('pointerShadow1').classList.add('pointer-down1');
    document.getElementById('pointer2').classList.add('pointer-down2');
    document.getElementById('pointerShadow2').classList.add('pointer-down2');
    document.getElementById('pointer3').classList.add('pointer-down3');
    document.getElementById('pointerShadow3').classList.add('pointer-down3');
    document.getElementById('pointer4').classList.add('pointer-down4');
    document.getElementById('pointerShadow4').classList.add('pointer-down4');
    document.getElementById('pointer5').classList.add('pointer-down5');
    document.getElementById('pointerShadow5').classList.add('pointer-down5');

Nie da się tego w jakąś pętle wrzucić?

---

 var timeout = timeout+4000;

zmienną timeout zdefiniowałeś już wyżej, teraz wystarczy tylko

timeout += 4000;

---

for (let i=0; i<arrWindows.length;  i++) {

Czy wiesz, dlaczego w tej linijce jest let?

---

setInterval("showSpeechBubbles()", 20000);

Nie wywołuj niepotrzebnie eval - eval to zło. Jeśli nie przekazujesz argumentów, po prostu:

setInterval(showSpeechBubbles, 20000);

---

Można również zastanowić się nad zamianą getElementBy* na CSS selector API. Dopóki nie wiesz co robisz - praca na "żywym HTMLu" nie jest najlepszym pomysłem.

 

PS dobrze, że zaczynasz bez jQ ; )

Comments

Dziękuje za rady.

Let powoduje, że zmienna staje się lokalną (jeżeli dobrze rozumiem) i w tym przypadku mogę ją przekazać do funkcji windowin oraz windowout.

Gdzie umieściłbyś zmienną h (wysokość okna)? Nie chcę z niej robić zmienną globalną.

 

window.addEventListener('scroll', runAnimationRun);
function runAnimationRun(){
    var h = Math.max(document.documentElement.clientHeight, window.innerHeight || 0);
    console.log(h);
    if (document.body.scrollTop > h || document.documentElement.scrollTop > h) {
        addShadows();
    }
}
// ......................... pointers and shadows...................
function addShadows() {
    for (x=1; x<5; x++) {
    document.getElementById('pointer'+x).classList.add('pointer-down'+x);
    document.getElementById('pointerShadow'+x).classList.add('pointer-down'+x);
    }
    var shadows = document.getElementsByClassName('fade-in');
    for(var i=0; i < shadows.length; i++) {
        shadows[i].classList.add('shadow-on');
    }
}
// ...................... Speech bubbles ..................
function showSpeechBubbles() {
    let arrWindows= document.getElementsByClassName("windows");
    var timeout = 4000;
    for (let i=0; i<arrWindows.length;  i++) {
            setTimeout(function windowin(){
                arrWindows[i].classList.add("showing");
            }, timeout);
            timeout += 4000;
            setTimeout(function windowout(){
                arrWindows[i].classList.remove("showing");
            }, timeout);
    }
}
showSpeechBubbles();
setInterval(showSpeechBubbles, 20000);

 

---

generalnie let oznacza dokładnie co piszesz, czyli stworzenie zmiennej w zasięgu blokowym a nie funkcyjnym, czyli odnoszącym się do nawiasów {} pętli for. Ale w tym wypadku jest również inna zaleta, a mianowicie w zapisie for (let i = 0, max = ...; i < max; i+=5){ } zmienna i jest deklarowana na nowo dla każdej iteracji pętli z wartością z ostatniej iteracji. Brzmi może nieco skomplikowanie ale pozwala pisać prostszy kod, jeśli w pętli for chcesz wywoływać jakąś funkcję operującą na i bez tworzenia dodatkowego "sztucznego" zasięgu oddzielną funkcją natychmiastową.

Pamiętaj jednak, że "let" to Ecma 6 i nie działa we wszystkich przeglądarkach.