DaftAcademy / frontend_levelup_2018

11 stars 33 forks source link

code review -> [flex] #22

Open michalwiacek opened 6 years ago

michalwiacek commented 6 years ago

Hej! tutaj jest link do mojego rozwiązania zadania 3. Rozumiem, że spieprzyłem Flexa, może ktoś rzucić okiem i powiedzieć co robie nie tak?

nekorushi commented 6 years ago

Cześć!

Już przystępuję do wyjaśnienia :)

Problem pierwszy:

image

Na klasie .Header ustawiony jest display: flex (co jest okej), natomiast potem element _.Header_logo jest pozycjonowany absolutnie. Z tego powodu użycie flexa nie daje żadnego efektu, bo złamana jest zależność między elementem flexowym a jego bezpośrednimi dziećmi.

Domyślam się, że takie rozwiązanie mogłeś zastosować np. nie mając pomysłu na wyśrodkowanie loga w poziomie, polecam zerknąć do przykładowego rozwiązania (https://daftcode.github.io/frontend_levelup_2018/), gdzie dałem pustego diva po prawej do zrównoważenia sekcji ze strzałką.

Problem drugi:

Element .LaunchDetails ma ustawione display: flex , żeby poukładać sekcje w kolumnę, ale jest to rozwiązanie nadmiarowe. Zwykłe nie-flexowe zachowanie elementów zrobiło by dokładnie to samo.

W przykładowym rozwiązaniu zastosowałem trochę podobną konstrukcję, ale w innym celu - żeby stopka aplikacji zawsze była przyklejona do dołu okna przeglądarki.

Element .page-container ustawiłem na display:flex i cały element ma minimalną wysokość 100% viewportu przeglądarki. Potem jego bezpośrednim dzieciom .page-content i .page-footer nadałem odpowiednio:

.page-content
    flex: 1

.page-footer
    flex: 0 0

Dzięki czemu stopka ma sztywną wysokość a zawartość strony rozpycha się maksymalnie (albo do wypełnienia strony, jeśli contentu jest mało, albo normalnie rośnie z contentem).

Problem trzeci:

image

Tutaj na element _.LaunchDetailsmain-container_ nadałeś display: flex, ale szerokość elementów _.LaunchDetailscounter i .LaunchDetails_details jest ustawiona na 50vw co w tej sytuacji nie jest potrzebne. Ustawienie flex: 1 na obu elementach spokojnie by wystarczyło i było by zgodne z konstrukcją flexa.

Z rzeczy mniejszych, ale na które warto zwrócić uwagę:

michalwiacek commented 6 years ago

dzięki za wyczerpującą odpowiedź! :)

DorotaSzostak commented 6 years ago

Hej, mam podobne pytanie, miałam odjęte punkty ze flex oraz BEM

https://github.com/DorotaSzostak/SpaceX-daftcode

majkamajka commented 6 years ago

@DorotaSzostak pozwolę sobie częściowo odpowiedzieć na twoje pytanie. jeżeli chodzi o BEM, to to, czego używasz jako modyfikator, tak naprawdę nie jest modyfikatorem - np. w header.sass masz element __arrow, i dodajesz do niego modyfikatory --pointer, --line i --text. to nie są modyfikatory a elementy. modyfikatorem do elementu __arrow byłyby np. klasy --active lub --disabled zmieniające np. kolor danego elementu.

nekorushi commented 6 years ago

@DorotaSzostak

Jeśli chodzi o BEMa: image

Problemów tutaj jest kilka:

  1. Punktację obniżyło Ci to co napisała @majkamajka, czyli modyfikatory użyte do oznaczenia elementów
  2. To nie wpływało na ocenę ale to taka uwaga ogólna: Elementy są podpięte pod klasę _.main_left co w realnej aplikacji narobiło by Ci problemów gdyby zmienił się projekt graficzny. Np. ktoś Ci kazał przenieść jakieś elementy na prawy panel i musiałabyś zmieniać nazwy klas dla wszystkich przenoszonych elementów, żeby zachować porządek. Ogólna rada jest taka, że klas nie nazywamy po tym jak one wyglądają, tylko do czego służą. Np. zamiast:
.main__left--date

Należało by dać

.main__launch-date

Wtedy jakbyś musiała to przerzucić do prawej kolumny, sprowadziło by się to jedynie do przestawienia elementu w HTMLu a CSS nie wymagałby poprawek.

Klasy _.mainleft i .mainright też niepotrzebnie implikują stronę po której się znajdują, można by obie nazwać tak samo, np. .main_column i ustawić im styl flex: 1 i one by się już fajnie ustawiły a ewentualna zamiana miejsc nie robiła by problemów.

Natomiast jeśli chodzi o flexa: image

Tutaj na elemencie .header ustawiony jest display: flex a _.headerarrow i .headerlogo mają szerokości ustawione na sztywno. Jak sobie zrobisz eksperyment i flex-basis przy .header_arrow zamienisz na width i całkiem zdejmiesz display: flex z .header to efekt będzie dokładnie ten sam, więc już się może zapalić czerwona lampka czy użycie flexa przy takim podejściu ma sens.

Podobnie tutaj: image Zwykłe ustawienie display: inline-block na elementach listy dało by ten sam efekt.

Ogólna rada to żebyś się zastanowiła co Ci w danym przypadku daje użycie flexa. Ja bardzo często zmieniam sobie rozmiar okna przeglądarki, żeby testować jak zmienia się zachowanie elementów na różnych szerokościach strony. Bardzo fajnie to może pomóc w zrozumieniu jak zachowują się elementy flexowe i nie-flexowe.

DorotaSzostak commented 6 years ago

dzięki za pomoc i rozjaśnienie kilku spraw :)

michalwiacek commented 6 years ago

Nie będę zakładał już drugiego wątku dla ostatniego zadania domowego, więc spytam tu. Co jest nie tak z moim ostylowaniem komponentu zgodnie z projektem graficznym, może coś pominąłem? https://michalwiacek.github.io/daftcode-spacex/

jakubgrzywaczewski commented 6 years ago

Jedyne do czego bym się przyczepił to font i to, że w buttonach tekst nie mieści się w jednym wierszu. Ale co ja tam wiem, u mnie to nie pomogło.

nekorushi commented 6 years ago

@michalwiacek

Z góry informuję, że przy ocenianiu tej pracy ze stylowaniem byłem znacznie surowszy, dlatego wyniki są wyraźnie niższe, ale nie ma się co martwić :) Ważne, żebyście starali się identyfikować błędy i je poprawiać.

Tym razem bardziej się czepiałem, gdy aplikacja źle zachowywała się przy różnych szerokościach ekranu, gdyż jak wiadomo, finalni użytkownicy korzystają z różnych urządzeń~

Ale przechodząc do rzeczy - było dość sporo niezgodności z projektem graficznym:

image

image

Robiąc layout pod projekt graficzny trzeba się starać, żeby aplikacja wyglądała schludnie niezależnie od wymiarów ekranu.

Oraz odległości między poszczególnymi kropkami są inne niż na wizualizacji: image

2560px szerokości: image

1440px: image

960px: image

image

To był błąd popełniany w 90% prac domowych, więc polecam się zapoznać z koncepcją Sticky Footera (np. tutaj jest fajny artykuł https://css-tricks.com/couple-takes-sticky-footer/)

Ogólnie chodzi o to, że kiedy zawartość twojej strony ma za małą wysokość, żeby wypełnić okno przeglądarki, to footer powinien być wypychany na dół strony a pozostała pusta przestrzeń nie była tak odcięta od naszego tła. Taki mechanizm zastosowałem za pomocą flexa w proponowanym rozwiązaniu poprzedniej pracy domowej (będzie też lepiej widoczny, gdy dostarczę przykładowe rozwiązanie):

image Gdzie .page-container ma ustawione:

display: flex;
flex-direction: column;
min-height: 100vh;

dzięki czemu zawsze ma minimum wysokość okna przeglądarki, .page-footer ma: flex: 0 0; co sprawia, że jest sztywny i nie zmienia swoich rozmiarów w ramach kolumny, natomiast .page-content ma flex: 1;, dzięki czemu jeśli nie ma wystarczająco dużo zawartości to rozpycha się i dociska footer na dół ekranu.

michalwiacek commented 6 years ago

Hej. Dzięki za wyczerpującą odpowiedź :). Szczerze mówiąc nie spodziewałem się RWD, przy wcześniejszych zadaniach była nawet mowa o tym, że nie będzie to oceniane. (chyba, że coś mi się pokićkało..) Stylowanie robiłem pod wyczką do chrome PixelPerfect, którą swoją drogą polecam.. Czy przy zadaniu następnym to stylowanie będzie ponownie oceniane?

nekorushi commented 6 years ago

Wiesz, to nawet nie jest RWD, bo to jest zwykle dopasowanie do desktopowych zakresów. Są gdzieś na świecie jeszcze ludzie, którzy korzystają z komputera na 1024x768, i dobrze jest zabezpieczyć stronę, żeby się w takiej sytuacji nie zjeżdżała. To samo przy dużych monitorach, jak ja mam np. 2560 px szerokości to strona zaprojektowana pod 1440 będzie bardzo nienaturalnie rozciągnięta. Warto jest wtedy ograniczyć wymiary witryny do jakichś rozsądnych ram i to bez stosowania RWD. W przykładowym rozwiązaniu z poprzedniej pracy domowej dałem takie ograniczenie przez co jak sobie zmniejszysz okno to w pewnym momencie strona zablokuje się na 960px i nie sprawia wrażenia zmiażdżonej a jak jest więcej niż 1440 to strona się ładnie środkuje i jest czytelna. A użyłem do tego max i min width ustawionego na divie, który opakowuje zawartość strony.

Przy tym sprawdzaniu pozwoliłem sobie poszaleć i czepiać się bardziej tak jak by to było w normalnej pracy, bo wtedy możecie zobaczyć, do jakich szczegółów musicie przykładać wagę. A takie uwagi dostalibyście nawet przy aplikacji, która z założenia nie ma być responsywna :)

NataD commented 6 years ago

@nekorushi Postanowiłam nieco zaszaleć i zadać Ci kilka pytań odnośnie Twojego komentarza.

Czy w normalnej pracy dostajesz wymagania w postaci spłaszczonego obrazka bez wymiarów, breakpointów oraz informacji jak się mają zachowywać elementy interfejsu na poszczególnych rozdzielczościach?

Czy w specyfikacji którą otrzymujesz widnieją błędy w postaci "NaN" przy letter spacing?

W którym miejscu w wymaganiach do zadania napisałeś, że projekt będziesz oceniał na przeróżnych szerokościach?

Takie rzeczy jak szerokość maksymalna czy też minimalna kontenera nie należą wyłącznie do decyzji front-endu. Ustala się to na etapie projektowania graficznego czy też makiety. Także uważam, że w tym przypadku powinny być spisanie w wymaganiach do projektu.

Myślę jednak, że jak Ty pracujesz nad projektami to masz nieco więcej informacji niż to co podałeś w treści zadania.

nekorushi commented 6 years ago

@NataD

Czy w normalnej pracy dostajesz wymagania w postaci spłaszczonego obrazka bez wymiarów, breakpointów oraz informacji jak się mają zachowywać elementy interfejsu na poszczególnych rozdzielczościach?

W przeważającej większości tak. Bardzo często się zdarza, że grafik robiąc wizualizację z braku czasu robi jedynie zgrubny projekt a masa szczegółów, jak np. zachowanie elementów trzeba doprojektować samodzielnie. Natomiast wymiary bez problemu można sobie zdjąć w dowolnym programie graficznym (nawet w Paincie), co regularnie muszę robić. Breakpointy z kolei bardzo często dopasowuje się na żywca testując co działa dobrze i posiłkując się ogólnie utartymi dobrymi praktykami.

Czy w specyfikacji którą otrzymujesz widnieją błędy w postaci "NaN" przy letter spacing?

U mnie NaNy naprawiało zwykłe odświeżenie strony. Ale nawet gdyby nie, to wystarczy puścić maila z informacją i wrzucilibyśmy poprawioną wersję.

W którym miejscu w wymaganiach do zadania napisałeś, że projekt będziesz oceniał na przeróżnych szerokościach?

Takiej informacji nie było ale też nie miało to dużego wpływu na ocenę (tak długo jak cała strona się nie sypała). W znaczącej większości ocenę obniżały złe wymiarowanie, niedopasowane kroje fontów i duże odchyły od głównego layoutu. Natomiast o zachowaniu strony na różnych szerokościach piszę dlatego, że jest to problem, na który trzeba zwracać uwagę praktycznie zawsze jak się implementuje layouty, po prostu przydatna wiedza.

Takie rzeczy jak szerokość maksymalna czy też minimalna kontenera nie należą wyłącznie do decyzji front-endu. Ustala się to na etapie projektowania graficznego czy też makiety. Także uważam, że w tym przypadku powinny być spisanie w wymaganiach do projektu.

W sumie tego nie zaznaczyłem a pewnie powinienem, ale ustawienie szerokości max/min było bardziej dobrą radą i nie było powodem obniżenia punktacji.

W normalnym flow produkcyjnym, gdybym zauważył taki problem i chciał wprowadzić takie rozwiązanie, to oczywiście najpierw było by to omówione z projektantem. Ale tak, zgadzam się - fajnie jest, kiedy takie rzeczy są definiowane w specyfikacji, ale najczęściej nie są i trzeba pracować z tym co się ma.

Myślę jednak, że jak Ty pracujesz nad projektami to masz nieco więcej informacji niż to co podałeś w treści zadania.

Treść zadania nie była mojego autorstwa i przykładowe rozwiązanie robiłem bazując w 100% na tych samych materiałach, które otrzymaliście wy. Materiały te są oszczędne i dają duże pole do interpretacji ale nie są one w żaden sposób oderwane od rzeczywistości ale w mojej opinii są absolutnie wystarczające, żeby przy zdroworozsądkowym podejściu ulepić z tego coś sensownego.

PS. Jeśli nie zgadzasz się z wystawioną oceną to podlinkuj swój projekt to podyskutujemy i uzasadnię, dlaczego oceniłem tak a nie inaczej.

michalwiacek commented 6 years ago

zadanie 5, czy trzeba doczytywać nowe biblioteki? podejście async await z prezentacji krzyczy: ReferenceError: regeneratorRuntime is not defined

przywartya commented 6 years ago

@michalwiacek Musisz sobie dociągnąć https://babeljs.io/docs/usage/polyfill/

krakus2 commented 6 years ago

Cześć Również chciałbym prosić o jakieś wskazówki co robię u siebie nie tak. Link do projektu - https://guarded-taiga-65836.herokuapp.com/ Link do repo - https://github.com/krakus2/spacex_react

Rozumiem dlaczego został mi odjęty punkt (a właściwie nie przyznany, bo miałem pustą lukę) za deploy na githubie, jednak było to dla mnie zaskoczeniem, ponieważ wszystkie poprzednie pracę również wrzucałem na heroku i nie było z tym żadnego problemu. Dodatkowo wrzuciłem swoją pracę domową na czas, może jest to jakiś argument za tym, żeby jednak ten punkcik mi przyznać :) (jednak abstrahując od punktów, uważam że heroku, nawet w darmowej wersji, jest lepszym miejscem do wrzucania swoich apek)

Co do stylowania, czytając poprzednie wyjaśnienia, widzę u siebie dwa błędy:

Został mi również odjęty punkt za podział na komponenty składowe. W poprzednim etapie miałem praktycznie identyczny podział i ktoś uznał, że jest to logiczne rozwiązanie. Co kuleje w tej pracy domowej?

Pozdrawiam i dzięki za poświęcony czas

michalwiacek commented 6 years ago

@nekorushi pisałeś o czcionkach, ale nie mogę znaleźć co jest nie tak.

nekorushi commented 6 years ago

@krakus2 Zaznaczam, że poniższe podpunkty są napisane na podstawie aktualnej wersji twojego projektu ale jeśli teraz specjalnie nie zmieniłeś tych elementów na niepoprawne, to moje podpunkty są aktualne :)

Kwestia tego punktu za deploy jest taka, że prace sprawdzamy w kilka osób i nie do końca wiedziałem jak tę sprawę traktować (trochę zawirowań urlopowych, różnie jest z komunikacją), ale nie mam problemu z przyznaniem za to punktów skoro aplikacja działa :)

Jeśli chodzi o stylowanko:

Wizualizacja image

U Ciebie image

Wizualizacja image

U Ciebie image

Wizualizacja image

U Ciebie image

Wizualizacja image

U Ciebie image

Ogólnie na niski wynik złożyła się duża ilość tych pomniejszych błędów.

Natomiast jeśli chodzi o punkt za podział na komponenty, to tutaj przeważyło powielenie kodu kolumn osi czasu. Jak sam pewnie zauważyłeś, kod komponentówListContainerLeft/ListContainerRight oraz ListBrickLeft/ListBrickRight jest powielony z minimalnymi zmianami w nazwach.

Wynikło z tego kilka problemów:

Lepszy efekt osiągnąłbyś uwspólniając kod tych komponentów i zrobił ListContainer oraz ListBrick a różnice między nimi zaimplementował używając propsów i modyfikatorów BEM'owych, np.:

<div className={`listBrick ${this.props.mirrored ? 'listBrick--right' : 'listBrick--left'}`}>
    <div className="listBrick__date">[...]</div>
    <img  className="listBrick__arrow"/>
    <div className="listBrick__arrow-line"></div>
    <div className="listBrick__circle"></div>
    <div className="listBrick__data">
        <span className="listBrick__data__rocket">ROCKET:</span>
        [...]
        <span className="listBrick__data__line">|</span>
        <span className="listBrick__data__launchSite">LAUNCH SITE:</span>
        [...]
    </div>
</div>

(pominąłem w twoim kodzie część tekstów i atrybutów, żeby nie zaciemniać struktury html'owej)

W takiej sytuacji miałbyś np. props na komponencieListBrick o nazwie mirrored, który ustawiałbyś na elementach po prawej stronie na true i zależnie od niego nadawana była by klasa z modyfikatorem --left albo --right (i tutaj byś miał perfekcyjny przykład na użycie modyfikatorów BEMowych). Różnice w wyglądzie lewej i prawej strony bez problemu da się rozwiązać samym CSSem a selektory byś budował właśnie na bazie tych modyfikatorów.

@michalwiacek W twoim projekcie teksty ogólnie są zbyt pogrubione

Wizualizacja image image

U Ciebie image image

Wizualizacja image

U Ciebie image

Ponadto teksty w stopce mają źle ustawione font-family przez co zamiast Open Sans wyświetla się chyba Times New Roman:

Wizualizacja image

U Ciebie image

Dzieje się tak, ponieważ zamiast font-family: "Open Sans", sans-serif, jak w pozostałych miejscach aplikacji, dałeś font-family: OpenSans. W sumie można by ustawić krój fontów bardziej globalnie, skoro w projekcie używany jest tylko jeden.

krakus2 commented 6 years ago

Cześć

Dziękuję bardzo za wyczerpującą odpowiedź, faktycznie się tego wszystkiego nazbierało. Ze wszystkim się oczywiście zgadzam, natomiast mam jedną małą uwagę co do funkcji skracającej nazwę miejsca startu. Nie wydaje mi się, że powinna być ona taka sama, ponieważ w podanej do zadania wizualizacji, starty po prawej stronie, są skracane dużo "szybciej" (przy mniejszej ilości znaków). Miejsce startu z prawej strony zostało skrócone do około 20 znaków, natomiast te z lewej mają koło 30, a dalej są pokazywane w pełnej krasie.

Pozdrawiam

nekorushi commented 6 years ago

@krakus2 Niby tak, ale różny moment ucinania tekstów nie wynika z tego, że grafik uznał "tu będzie dłuższe, tu będzie krótsze" a z różnicy w układzie elementów na lewej i prawej kolumnie. W zasadzie to nigdy się nie definiuje takich rzeczy na sztywno, bo te wartości musiały by być inne dla różnych szerokości ekranu i wielu innych czynników.

Ja to widzę tak, że prawa wersja kolumny w projekcie ma wymuszony pewien sztywny margines od strony osi, żeby zrobić iluzję symetrii względem kolumny lewej. I ucinanie tekstu nie jest zdefiniowane konkretną liczbą znaków a bardziej maksymalną szerokością, jaką może zająć.

A pamiętaj, że różne litery mają różne szerokości - trochę ekstremalny przykład, ale podmieniłem sobie nazwę launch site'a w twojej aplikacji na szereg szerokich znaków i efekt już jest dość kiepski: image

Zakładając, że dane z backendu mogą przyjść najróżniejsze, musisz przygotowywać front tak, żeby był w stanie obsłużyć każdą, nawet najbardziej ekstremalną sytuację.

Cały ten problem można by rozwiązać znacznie prościej za pomocą CSSa. Gdybyś zamiast przepuszczać ten tekst przez funkcję opakował go w HTMLu w znacznik, np. <span>, to mógłbyś mu nadać style:

text-overflow: ellipsis;
overflow: hidden;

Aplikacja sama sobie zadecyduje, ile tekstu zmieści się w zadanej przestrzeni i wyświetli maksymalnie dużo tekstu do momentu kiedy to jeszcze wygląda dobrze, a potem sobie utnie: image image

Ale nawet gdybyś się uparł na użycie tej funkcji, to mógłbyś uwspólnić kod tych komponentów parametryzując ją, np.:

shortName = (data, limit = 32) => {
    if(data.length > limit + 3 && data.charAt(limit - 1) !== ' '){
      return `${data.slice(0, limit)}...`
    } else if(data.length > limit) {
        return `${data.slice(0, limit - 1)}...`
    } else {
        return data
    }
  }

(funkcję modyfikowałem na dziko, bez zagłębiania się w jej działanie ale zasada parametryzacji była by z grubsza ta sama)

Wtedy mógłbyś w zależnie od np. propsa "shortNameLimit" czy czegoś podobnego konfigurować, kiedy tekst jest ucinany, a logika funkcji nie była by powielona.

krakus2 commented 6 years ago

Jeszcze raz dziękuję bardzo za wszystkie rady i sugestie. Poprawiłem według Waszych wskazówek i wyszło mi coś takiego:

nekorushi commented 6 years ago

@krakus2 Spora poprawa, czcionki faktycznie za duże, w specyfikacji obok grafik jest wykaz krojów z rozmiarami i grubościami. Ktoś wcześniej wspominał, że nie zawsze się wyświetlają, to łap screenshota: image

Ponadto zauważyłem, że wprowadziłeś niepoprawne nazewnictwo klas, tzn. niezgodne z BEMem. Zadaniem BEMa nie jest oddanie struktury HTMLowej a jedynie pewnej relacji między głównym elementem komponentu i elementami podrzędnymi. Dlatego też nie zagnieżdżamy elementów BEMowych.

Zatem np. listBrick__data__launchSite2 nie jest poprawne. Wystarczyłoby, gdybyś ustanowił element listBrick__data a elementy w środku nazwał listBrick__data-launchSite2 (jeśli uważasz, że ich obecność jest ściśle powiązana z listBrick__data) albo listBrick__launchSite2 (jeśli w hipotetycznej przyszłości czułbyś, że ten element można by wykorzystywać w innych miejscach elementu listBrick, nie tylko wewnątrz listBrick__data)

Coś na ten temat w oficjalnych źródłach - polecam przeczytać: http://getbem.com/faq/#css-nested-elements

nekorushi commented 6 years ago

@krakus2 Pokombinuj też jeszcze z tymi kropkami i strzałkami, bo rozjeżdżają się na różnych szerokościach a to od razu pokazuje, że coś z obecnym podejściem jest nie tak (nie bój się tego zaorać i spróbować ostylować od nowa, czasami znając już problemy jakie mogą wystąpić, zaprojektowanie nowego rozwiązania na czysto sporo pomaga)

Będę starał się przygotować proponowane rozwiązanie tej pracy domowej, tylko muszę znaleźć trochę wolnego czasu. Będziecie mogli zobaczyć, jak można fajnie niektóre efekty osiągnąć.