-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
code review -> [flex] #22
Comments
Cześć! Już przystępuję do wyjaśnienia :) Problem pierwszy: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.
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:Tutaj na element .LaunchDetails__main-container nadałeś display: flex, ale szerokość elementów .LaunchDetails__counter 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ę:
|
dzięki za wyczerpującą odpowiedź! :) |
Hej, mam podobne pytanie, miałam odjęte punkty ze flex oraz BEM |
@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 |
Problemów tutaj jest kilka:
Należało by dać
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 .main__left i .main__right 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: Tutaj na elemencie .header ustawiony jest display: flex a .header__arrow i .header__logo 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: 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. |
dzięki za pomoc i rozjaśnienie kilku spraw :) |
Nie będę zakładał już drugiego wątku dla ostatniego zadania domowego, więc spytam tu. |
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. |
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:
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:
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):
dzięki czemu zawsze ma minimum wysokość okna przeglądarki, |
Hej. Dzięki za wyczerpującą odpowiedź :). |
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 :) |
@nekorushi 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. |
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.
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ę.
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.
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.
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. |
zadanie 5, czy trzeba doczytywać nowe biblioteki? podejście async await z prezentacji krzyczy: |
@michalwiacek Musisz sobie dociągnąć https://babeljs.io/docs/usage/polyfill/ |
Cześć 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 |
@nekorushi pisałeś o czcionkach, ale nie mogę znaleźć co jest nie tak. |
@krakus2 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:
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ów Wynikło z tego kilka problemów:
Lepszy efekt osiągnąłbyś uwspólniając kod tych komponentów i zrobił
(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 komponencie @michalwiacek
Ponadto teksty w stopce mają źle ustawione Dzieje się tak, ponieważ zamiast |
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 |
@krakus2 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: 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.
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: Ale nawet gdybyś się uparł na użycie tej funkcji, to mógłbyś uwspólnić kod tych komponentów parametryzując ją, np.:
(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. |
Jeszcze raz dziękuję bardzo za wszystkie rady i sugestie. Poprawiłem według Waszych wskazówek i wyszło mi coś takiego:
|
@krakus2 Ponadto zauważyłem, że wprowadziłeś niepoprawne nazewnictwo klas, tzn. niezgodne z BEMem. Zatem np. Coś na ten temat w oficjalnych źródłach - polecam przeczytać: http://getbem.com/faq/#css-nested-elements |
@krakus2 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ąć. |
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?The text was updated successfully, but these errors were encountered: