Carousel #1

wisnie · 2019-12-27T11:39:01Z

Pull Request to master

typeofweb · 2019-12-27T11:47:06Z

index.html

+        </div>
+        <div class="container">
+            <div class="carousel">
+                <button class="button button--left">


Rzeczy, które nie działają bez JS powinny zostać dodane dopiero przez skrypt.
A więc:

przyciski

kropeczki

klasy "active"

itp.

Hasło: Progressive Enhancement

typeofweb · 2019-12-27T11:47:53Z

index.html

+                        style="width:24px;height:24px"
+                        viewBox="0 0 24 24"
+                    >
+                        <path


Ten przycisk nie ma żadnej treści, a więc będzie całkowicie niedostępny dla czytników ekranowych

typeofweb · 2019-12-27T11:48:12Z

index.html

+                            class="carousel__image"
+                        />
+                    </div>
+                    <div class="carousel__slide carousel__slide--active">


Tę klasę powinien dodać skrypt

typeofweb · 2019-12-27T11:48:25Z

index.html

+                    </svg>
+                </button>
+            </div>
+            <div class="dotsNav">


Te kropki powinien generować skrypt

typeofweb · 2019-12-27T11:48:42Z

index.html

+                </button>
+                <div class="carousel__container">
+                    <div class="carousel__slide">
+                        <img


Moim zdaniem to pierwsze zdjęcie powinno być aktywne na początku, a nie trzecie.

to akurat moja wina :) - muszę poprawić na makiecie

typeofweb · 2019-12-27T11:56:38Z

src/main2.js

+    });
+
+    //Swipe Events
+    document.addEventListener('touchstart', handleTouchStart, false);


Czy nie lepiej byłoby użyć Pointer Events?

typeofweb · 2019-12-27T11:58:05Z

src/main2.js

+
+        if (xDirection > 0) {
+            moveToLeft();
+        } else if (xDirection < 0) {


Tutaj powinna być pewna wartość graniczna, np. 20 pikseli

typeofweb · 2019-12-27T11:58:21Z

src/main2.js

+
+    //Swipe Events
+    document.addEventListener('touchstart', handleTouchStart, false);
+    document.addEventListener('touchmove', handleTouchMove, false);


Jeśli dobrze rozumiem, to taki kod spowoduje, że lekkie drgnięcie palca w złą stronę spowoduje przesunięcie karuzeli.

typeofweb · 2019-12-27T11:58:45Z

src/styles.css

@@ -0,0 +1,218 @@
+@import url('https://fonts.googleapis.com/css?family=Poppins:400,600&display=swap');


Lepiej umieścić to w pliku index.html

typeofweb · 2019-12-27T11:59:24Z

src/styles.css

+    width: 40px;
+}
+
+@media (max-width: 1400px) {


Dlaczego nie mobile-first?

czechue · 2019-12-27T12:06:24Z

src/main2.js

+    slides.forEach(slide => {
+        slide.addEventListener('click', () => {
+            //Middle Slide is 3rd position (0,1,2) from left, so we need to subtract two.
+            if (isInTransition === true) return;


wystarczy
if (isInTransition) { return ; }
jestem za tym abyś dodawał nawiasy { ... } za ifem

czechue · 2019-12-27T12:28:54Z

index.html

+                            class="carousel__image"
+                        />
+                    </div>
+                    <div class="carousel__slide">


te slajdy jednak powinny być renderowane za pomocą JS.
jest tu bardzo dużo powtórzeń kodu

czechue · 2019-12-27T12:31:49Z

src/main2.js

+    let elementWidth = slides[0].offsetWidth;
+    let isInTransition = false;
+    //Middle Dot
+    let activeDot = 3;


wszelkie wartości "experckie" / "defaultowe" czy z innego względu magiczne warto zapisywać uppercasem:
const DEFAULT_ACTIVE_DOT = 3;
let activeDot = DEFULT_ACTIVE_DOT;

czechue · 2019-12-27T12:32:18Z

src/main2.js

+const carousel = document.querySelector('.carousel__container');
+
+window.addEventListener('load', () => {
+    let slides = document.querySelectorAll('.carousel__slide');


dlaczego let a nie const?

czechue · 2019-12-27T12:41:14Z

src/main2.js

+    const moveToRight = () => {
+        if (isInTransition === true) return;
+        carousel.appendChild(carousel.firstElementChild);
+        slides = carousel.querySelectorAll('.carousel__slide');


może do osobnej funkcji:

getSlides(carousel) { let element = carousel; element.appendChild(carousel.firstElementChild); return element.querySelectorAll('.carousel__slide') }

wtedy nie mutujesz elementów (ale nie wiem czy to tak ma działać u Ciebie)

czechue · 2019-12-27T12:44:36Z

src/main2.js

+        setActiveElement(slides);
+    };
+
+    const moveToLeft = () => {


te funkcje wykonują bardzo dużo side-effectów.
nie są zupełnie testowalne, mutują obiekty, pobierają dane ze scopa.
ich odpowiedzialność jest bardzo duża i rozmyta.

starałbym się pisać więcej małych pure funkcji, które przyjmują argumenty i zwracają jakąś wartość.

czechue · 2019-12-29T12:03:59Z

src/main2.js

@@ -0,0 +1,272 @@
+window.addEventListener('load', () => {


czy ten load jest w ogóle potrzebny?
imho nie :)

czechue · 2019-12-29T12:07:37Z

src/main2.js

+        'afterbegin',
+        '<svg class="container__svg" style="width:24px;height:24px" viewBox="0 0 24 24"><path class="container__path" fill="#000000"  d="M8.59,16.58L13.17,12L8.59,7.41L10,6L16,12L10,18L8.59,16.58Z"/></svg> <p class="screen-readers">Right button for moving photos.</p>'
+    );
+    carousel.appendChild(buttonRight);


wszystkie funkcje odpowiedzialne za tworzenie elementów DOM przeniósłbym do swoich osobnych plików i tylko importował tutaj:

export const renderButton(side) { const button = document.createElement('button'); button.classList.add('button'); button.classList.add(`button--${side}`); ... i tu lepiej skorzystać z insertAdjacentHTML niż tak dodawać po kolei atrybuty }

i wtedy w tym pliku importujesz:
import {renderButton} from './renderButton'

dodatkowo masz funkcje renderButtonLeft i renderButtonRight -> aż się prosi o dodanie parametru jako argument w tej funkcji

czechue · 2019-12-29T12:08:01Z

src/main2.js

+        );
+        buttonLeft.classList.toggle('button--hidden');
+        buttonRight.classList.toggle('button--hidden');
+    });


cały ten arrowSwitch -> do osobnego pliku jak buttony.
i łatwiej byłoby jak byś stworzył całe ten html jako string i skorzystał z insertAdjacentHTML

czechue · 2019-12-29T12:09:34Z

src/main2.js

+        carouselContainer.appendChild(div);
+    };
+
+    const images = document.querySelectorAll('.carousel__image');


wszystkie selektory przeniósłbym na górę pliku w 1 miejsce

czechue · 2019-12-29T12:31:07Z

src/main2.js

+    const buttonRight = document.createElement('button');
+    buttonRight.classList.add('button');
+    buttonRight.classList.add('button--right');
+    buttonRight.insertAdjacentHTML(


dlaczego całego buttona nie stworzysz za pomocą insertAdjacentHTML:
const button = <button class="button button--${side}"><svg>... </button>
bez sensu dodawać te klasy za pomocą classList.add można to zrobić w stringu

czechue · 2020-01-03T19:54:22Z

src/carousel.js

+    return subtractionOfMultipliers;
+};
+
+const isApplicableForAddingClass = slide => {


jeśli funkcja ma zwracać booleana - to powinna zwracać true lub false
tutaj zwraca true albo nic :)

const isApplicableForAddingClass = slide => { return checkMultipliersDiffrence(slide) === 0; }

zwróci booleana

czechue · 2020-01-03T19:59:40Z

src/carousel.js

+    }
+};
+
+const isOneMove = multipliersDiffrence => {


return multipliersDiffrence < 2 && multipliersDiffrence > -2

czechue · 2020-01-03T20:00:04Z

src/carousel.js

+    }
+};
+
+const isApplicableForMove = slide => {


return checkMultipliersDiffrence(slide) !== 0

czechue · 2020-01-03T20:00:48Z

src/carousel.js

+    }
+};
+
+const isActive = slide => {


return slide.classList.contains('carousel__slide--scale')

czechue · 2020-01-03T20:03:17Z

src/renderSwitch.js

+        '.arrows-switch__container'
+    );
+
+    arrowsSwitchContainer.addEventListener('click', () => {


truchę mi ten listener tutaj nie pasuje w tej funkcji

larto42 · 2020-01-03T19:14:46Z

index.html

+            </div>
+            <div class="dotsNav"></div>
+        </div>
+        <script defer src="src/carousel.js"></script>


script defer powinien chyba być w headzie. Jak dasz na końcu body to wydaje mi się, że zanim zacznie się pobierać to cały html się załaduje i nie bedziesz miał oczekiwanego efektu pobierania pliku podczas przetwarzania html.

larto42 · 2020-01-03T19:33:24Z

src/carousel.js

+const carouselRightButton = document.querySelector('.button--right');
+const carouselLeftButton = document.querySelector('.button--left');


jeśli elementy zawierają się w carouselContainer, to można ograniczyć selector do tego elementu

larto42 · 2020-01-03T19:37:57Z

src/carousel.js

+};
+
+const updateCurrentActiveSlide = slides => {
+    const middleElement = (maxCountOnScreen + 1) / 2;


maxCountOnScreen może być parzyste? jeśli tak to (maxCountOnScreen + 1) / 2 zwróci ułamek, który bezkrytycznie użyjesz jako index tablicy

larto42 · 2020-01-03T19:40:36Z

src/carousel.js

+    if (direction === 'left') {
+        activeDot === 0 ? (activeDot = slides.length - 1) : activeDot--;
+    } else if (direction === 'right') {
+        activeDot === slidesCounter - 1 ? (activeDot = 0) : activeDot++;


może activeDot = activeDot === 0 ? slides.length - 1 : activeDot - 1; ?
nie ładniej będzie? od razu widać jaką zmienną updatujesz i nie potrzeba nawiasów w środku wrzucać

larto42 · 2020-01-03T19:41:13Z

src/carousel.js

+let dots = document.querySelectorAll('.dotsNav__dot');
+let elementWidth = slides[0].offsetWidth;
+let isInTransition = false;
+let activeDot = 0;


Wyciągnąć do stałej

larto42 · 2020-01-03T20:08:19Z

src/carousel.js

+}
+
+function handleTouchMove(event) {
+    if (!yStart || !xStart) {


wydaje mi się czy nie zadeklarowałeś yStart?

larto42 · 2020-01-03T20:10:10Z

src/carousel.js

+pageBackground.addEventListener('click', () => {
+    if (!isInTransition) {
+        if (carouselContainer.querySelector('.carousel__slide--scale')) {
+            carouselContainer
+                .querySelector('.carousel__slide--scale')
+                .classList.remove('carousel__slide--scale');
+            pageBackground.classList.remove('pageBackground--scale');
+        }
+    }
+});


listenery, funkcje, listenery, wywołania funkcji (niżej), listenery.
można pogrupować?

larto42 · 2020-01-03T20:11:40Z

src/renderButton.js

+    right:
+        '<svg class="container__svg" style="width:24px;height:24px" viewBox="0 0 24 24"><path class="container__path" fill="#000000"  d="M8.59,16.58L13.17,12L8.59,7.41L10,6L16,12L10,18L8.59,16.58Z"/></svg>',
+    left:
+        '<svg class="container__svg" style="width:24px;height:24px" viewBox="0 0 24 24"><path class="container__path" fill="#000000" d="M15.41,16.58L10.83,12L15.41,7.41L14,6L8,12L14,18L15.41,16.58Z"/></svg>'


width i height możesz spokojnie w css sobie wrzucić. viewBox i tak zadba o dobre proporcje

larto42 · 2020-01-03T20:14:54Z

src/styles.css

+.screen-readers {
+    position: absolute;
+    right: 2400px;
+}


opowiesz coś o tym? nie pisałem dla screen readerów więc może coś się dowiem ciekawego, ale na 1 rzut oka dziwnie mi to wygląda.

larto42 · 2020-01-03T20:16:06Z

src/styles.css

+.container__svg {
+    width: 50px !important;
+    height: 50px !important;
+}


o jak przeniesiesz width i height z svg tu, to nie będziesz potrzebował !important

czechue · 2020-01-05T18:21:10Z

src/renderButton.js

+};
+
+renderButton('left');
+renderButton('right');


mi się generalnie nie podoba za bardzo idea wywoływania tych funkcji w tych komponentach.
powinieneś exportować funkcję renderButton importować ją w jakimś głównym module i tam ją wywoływać

w ten sposób możesz też przekazywać scope z głownego modułu:

export default function renderButton(carouselElement, side) => ... // index.js import renderButton from './renderButton'; const carousel = document.querySelector('.carousel'); renderButton(carousel, 'left') renderButton(carousel, 'right')

wtedy te funkcje są pure i znacznie bardziej reużywalne

czechue · 2020-01-05T18:21:52Z

src/renderDots.js

+    }
+};
+
+renderDots(slidesCounter);


j.w. w renderButton.js

czechue · 2020-01-05T18:22:23Z

src/renderPageBackground.js

+    <div class="pageBackground"></div>
+`;
+
+document.body.insertAdjacentHTML('afterbegin', pageBackground);


czechue · 2020-01-05T18:22:36Z

src/renderSlides.js

+    carouselContainer.appendChild(div);
+};
+
+images.forEach(image => generateSlide(image));


czechue · 2020-01-05T18:23:13Z

src/renderSwitch.js

+        </div>
+    `;
+
+    document


j.w.
wywoływanie tej funkcji powinno być w głównym module

Carousel

a08be11

typeofweb suggested changes Dec 27, 2019

View reviewed changes

czechue reviewed Dec 27, 2019

View reviewed changes

wisnie added 2 commits December 27, 2019 20:43

Carousel with Quickfix

ffa0c19

Fixes

54a7760

czechue reviewed Dec 29, 2019

View reviewed changes

src/main2.js Outdated

@@ -0,0 +1,272 @@

window.addEventListener('load', () => {

Copy link

czechue Dec 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

czy ten load jest w ogóle potrzebny?
imho nie :)

czechue reviewed Dec 29, 2019

View reviewed changes

wisnie and others added 4 commits December 29, 2019 17:19

Another fix

9e071c9

Add files via upload

a9a39a5

Add files via upload

a2870b6

Add files via upload

d5430c3

czechue reviewed Jan 3, 2020

View reviewed changes

src/carousel.js

}

};

const isOneMove = multipliersDiffrence => {

Copy link

czechue Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return multipliersDiffrence < 2 && multipliersDiffrence > -2

czechue reviewed Jan 3, 2020

View reviewed changes

src/carousel.js

}

};

const isApplicableForMove = slide => {

Copy link

czechue Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return checkMultipliersDiffrence(slide) !== 0

czechue reviewed Jan 3, 2020

View reviewed changes

src/carousel.js

}

};

const isActive = slide => {

Copy link

czechue Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return slide.classList.contains('carousel__slide--scale')

czechue reviewed Jan 3, 2020

View reviewed changes

larto42 reviewed Jan 3, 2020

View reviewed changes

Add files via upload

36a6361

czechue reviewed Jan 5, 2020

View reviewed changes

src/renderDots.js Outdated

}

};

renderDots(slidesCounter);

Copy link

czechue Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

j.w. w renderButton.js

czechue reviewed Jan 5, 2020

View reviewed changes

src/renderPageBackground.js

<div class="pageBackground"></div>

`;

document.body.insertAdjacentHTML('afterbegin', pageBackground);

Copy link

czechue Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

j.w.

czechue reviewed Jan 5, 2020

View reviewed changes

src/renderSlides.js Outdated

carouselContainer.appendChild(div);

};

images.forEach(image => generateSlide(image));

Copy link

czechue Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

j.w.

czechue reviewed Jan 5, 2020

View reviewed changes

src/renderSwitch.js

</div>

`;

document

Copy link

czechue Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

j.w.
wywoływanie tej funkcji powinno być w głównym module

Add files via upload

41b1025

wisnie merged commit 0f60d0a into master Jan 7, 2020

		@@ -0,0 +1,218 @@
		@import url('https://fonts.googleapis.com/css?family=Poppins:400,600&display=swap');

		const carouselRightButton = document.querySelector('.button--right');
		const carouselLeftButton = document.querySelector('.button--left');

Carousel #1

Carousel #1

Conversation

wisnie commented Dec 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czechue Dec 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czechue Dec 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czechue Dec 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czechue Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czechue Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czechue Dec 27, 2019 •

edited

Loading

czechue Dec 29, 2019 •

edited

Loading

czechue Dec 29, 2019 •

edited

Loading

czechue Jan 3, 2020 •

edited

Loading

czechue Jan 3, 2020 •

edited

Loading