-
Notifications
You must be signed in to change notification settings - Fork 1
zmiana #1
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
base: master
Are you sure you want to change the base?
zmiana #1
Conversation
nav.css
Outdated
| * { | ||
| box-sizing: border-box; | ||
| } | ||
| body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, na początek musimy zaprowadzić porządek w kodzie. W naszych stylach selektor body występuje w kilku miejscach - mało tego, w trzech ustawiamy margin: 0 oraz padding: 0, a w dwóch font-family: 'Oxygen', sans-serif;. Oczywiście każdą właściwość wystarczy zadeklarować raz :) te deklaracje oprócz tego miejsca pojawiają się jeszcze w plikach reset.css oraz style.css. Oczywiście uznałbym, że tutaj czyli w nav.css nie musimy zamieszczać stylowania dotyczącego tagu body, więc stąd na pewno bym to skasował. Co do margin:0 i padding: 0 robimy to, żeby ujednolicić tak jakby wyjściowe style dla elementu body ponieważ niektóre przeglądarki potrafią dodawać własne bazowe style do niektórych elementów (np. input bez naszych styli jest szary i prostokątny, ma border itp. - podobnie body czasami dostaje padding i margin), takie rzeczy resetujemy w pliku reset.css i tam bym zostawił ten margin i padding. Po drugie deklarujemy tutaj też font-family, który jest już całkiem specyficzny dla naszej strony, więc to już w pliku reset.css znajdować się nie powinno. I tą deklarację umieściłbym w pliku style.css.
nav.css
Outdated
| width: 300px; | ||
| padding-left: 30px; | ||
| } | ||
| .navbar-branding a { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Używanie takich klas jak navbar czy navbar-branding jest dobrą praktyką, bo oba elementy należą do jednego komponentu (łatwiej się połapać w kodzie). Ograniczałbym jednak stylowanie po tagach jak w tym przypadku. Zwłaszcza, że ten element ma na sobie klasę logo1, której możemy użyć. A nawet bym ją zmienił na coś w stylu navbar-logo dzięki czemu trzymamy się konwencji i traktujemy ten element jako podelement nawigacji... albo po prostu klasy .logo i wtedy element traktujemy jako oddzielny komponent, bo może ten sam element (logo), będziemy chcieli umieścić gdzieś poza nawigacją - wystarczy wtedy skorzystać z tej klasy. Poza tym im krótsze selektory tym lepiej. Nie ma konieczności tego tak zagnieżdżać. Tutaj wystarczy selektor składający się z samej klasy zamiast klasy i tagu.
nav.css
Outdated
| color: white; | ||
| } | ||
| .logo2::after { | ||
| content: "market"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zawsze fajnie wiedzieć, że można coś takiego zrobić, ale mimo wszystko unikałbym wstawiania w content jakichkolwiek słów :P jest to niesemantyczne, nie po to zostały zaprojektowane CSSy. Do wyświetlania treści służy HTML i tego trzeba się trzymać. Wszelkie napisy umieszczone w content nie zawsze zachowują się tak jakbyśmy chcieli, trudniej je edytować itp.
index.html
Outdated
| <a href="" class="logo2">envato</a> | ||
| </div> | ||
| <div class="navbar-list"> | ||
| <div><a href="" class="logo3">buy now</a></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pytanie czy musimy opakowywać a w div? Dlaczego chcemy to zrobić? Czy wiemy dlaczego po opakowaniu w div coś nagle zaczyna działać tak jakbyśmy chcieli? ;) Nie jest to jakiś błąd, ale załóżmy, że chcemy pozbyć się divów obejmujących elementy logo3 oraz logo4 - co musimy zrobić, żeby inaczej wyśrodkować w pionie oba elementy? ;) (w niektórych przypadkach opakowanie coś w div ma sens, ale w tym przypadku jest to raczej zbędne)
nav.css
Outdated
| width: 50%; | ||
| } | ||
| .navbar-branding { | ||
| width: 300px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, mamy elementy .navbar-branding oraz .navbar-list. Jeden ma 300px szerokości a drugi 50% przez co powstaje pewien mix. navbar-list dopasowuje się do okna przeglądarki, ponieważ zajmuje procentową część szerokości nawigacji, a navbar-branding pozostaje zawsze sztywny i szeroki na 300px - ale czysto teoretycznie, ponieważ display: flex zawsze będzie dążył do tego aby oba elementy znajdowały się w rzędzie. W pewnym więc sensie możemy powiedzieć, że obie te wartości są tutaj zbędne, bo i tak będą się trzymać w rzędzie. A odsuwamy je od siebie już za pomocą justify-content: space-around;
PS to samo dotyczy nawigacji niżej.
PS2 stopka reprezentuje się bardzo dobrze na tle nawigacji
nav.css
Outdated
| align-items: center; | ||
| } | ||
| .navbar-list { | ||
| display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zadanie na szóstkę
umieśćmy w rzędzie elementy wewnątrz navbar-list bez używania display: flex
nav.css
Outdated
| color: black; | ||
| height: 80px; | ||
| display: flex; | ||
| flex-direction: row; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co się stanie jeżeli skasujemy tutaj tą właściwość? Nic. Dlaczego? Niektóre właściwości mają swoje bazowe wartości, np.
<div> ma display: block
<span> ma display: inline
<a> ma text-decoration: underline
podobnie flex-direction bazowo ma wartość row - zatem nie ma potrzeby deklarować jej jeszcze raz. Swoją drogą to właśnie dlatego jeżeli dajemy elementowi właściwość display: flex automatycznie wszystkiego jego bezpośrednie dzieci układają się w rzędzie.
header.css
Outdated
| font-weight: 900; | ||
| letter-spacing: 2px; | ||
| z-index: 2; | ||
| position: absolute; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pozycjonowanie rzeczy absolutnie ogólnie jest dość niebezpieczne i najlepiej uciekać się do tego tylko kiedy zawodzą wszelkie inne metody. Wartości określone w top left right czy bottom są dość trudne do utrzymania jeżeli w grę wchodzi responsywność. Zawsze należy dążyć do tego aby elementy w HTML zachowywały się jak najbardziej naturalnie. Układały się obok, pod sobą itp. o wiele łatwiej jest tym manipulować i przewidzieć co może się stać kiedy np. tekst w paragrafie się zwiększy. W przypadku elementów pozycjonowanych absolutnie często mamy do czynienia z problemem, że np. tekst nachodzi na przycisk czy ogólnie jakiś element wyjechał poza okno przeglądarki i nie jesteśmy w stanie już go nawet zobaczyć. Spróbujmy ostylować nasz główny header bez użycia position: absolute
header.css
Outdated
|
|
||
| } | ||
| .linia1{ | ||
| color: white; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, linia1 oraz linia2 mają dokładnie te same style. Zawsze powinniśmy dążyć do tworzenia jak najmniejszej ilości kodu. Moglibyśmy skrócić oba zapisy i połączyć w jedno. Jak? ;)
index.html
Outdated
| <div class="linia"> | ||
| <h1> | ||
| <span class="linia1">bootstop</span> | ||
| <span class="linia2 color">carouse</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
czy używamy gdzieś klasy color ?
section.css
Outdated
| @@ -0,0 +1,66 @@ | |||
| .card-section { | |||
| max-width:100%; | |||
| max-height: 100%; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Czy potrzebujemy obu powyższych właściwości?
section.css
Outdated
| .card { | ||
| padding: 10px; | ||
| position: relative; | ||
| background-repeat: no-repeat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Czy potrzebujemy tutaj tej właściwości? background-repeat jest ściśle powiązany z background-image, którego ten element nie ma, zatem i ta właściwość jest zbędna.
index.html
Outdated
|
|
||
| <header> | ||
| <main class="color"> | ||
| <h1>recent works</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Na stronie może występować tylko jeden element h1 determinujący najważniejszy nagłówek. Reszta, czyli h2, h3 itd. mogą występować już w większych ilościach, ale warto tak konstruować HTML aby wiedzieć jaki nagłówek jest ważniejszy, a który mniej, żeby stworzyć ich odpowiednią hierarchię.
section.css
Outdated
| font-size: 16px; | ||
| line-height: 1.5; | ||
| text-align: justify; | ||
| line-height: 1.8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dwie linijki wyżej zadeklarowaliśmy już line-height - musimy się zdecydować na jedną wartość.
footer.css
Outdated
| .tekst1 { | ||
| padding: 0 10px; | ||
| } | ||
| .tekst2:hover { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Czy jest jakaś różnica w działaniu cursor: pointer dla samego tekst2 i tekst2:hover? Kursor zmienia się dopiero po najechaniu na element, wcześniej nie jest to fizycznie możliwe. Utrzymuje się, że dla cursor najbardziej odpowiedni będzie selektor bez pseudoklasy hover.
ourcontact.css
Outdated
| .ourcontact1 p { | ||
| padding: 15px 0; | ||
| font-size: 15px; | ||
| color: black; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wszystkie elementy w ourcontact1 mają kolor czarny, ale każdemu elementowi nadajesz go oddzielnie. Dla p h1 i h3. Jedną z zasad CSS jest DRY - Don't repeat yourself czyli nie powtarzaj się w kodzie. Wszystkie powyższe deklaracje właściwości color: black można ukrócić do jednego zapisu. W tym celu warto zapoznać się zagadnieniem dziedziczenia w CSS
http://webkod.pl/kurs-css/lekcje/dzial-1/dziedziczenie-wlasciwosci-css
kontakt.html
Outdated
| <p>support@yourmail.com</p> | ||
| </div> | ||
| <div class="mapa"> | ||
| <img src="mapa.jpg" class="mapa" alt="lokalizacja"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obrazek jak i rodzic mają tą samą klasę. Czy na pewno tego chcemy?
kontakt.html
Outdated
|
|
||
| <div class="contact-form"> | ||
| <div class="contakt-form-inner"> | ||
| <form action="" method=""> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, mamy tutaj lekki nieporządek. Ten form nie ma domknięcia. Obok jest drugi form z elementem textarea, w którym domknięcia też się trochę pokręciły.
Oprócz tego nasz formularz kontaktowy powinniśmy potraktować jako jeden a nie dwa formularze. Wciskając "Send message" chcemy wysłać jeden formularz a nie dwa, prawda? Tak więc nasz pierwszy form powinien zawierać w sobie także textarea. Warto też wiedzieć, że na elementy formularza składają się nie tylko inputy i textarea, ale też np. przycisk, którym chcemy wysłać dany formularz (w naszym przypadku przycisk "send message")
contact-form.css
Outdated
| padding: 18px 50px 80px 10px; | ||
| text-transform: capitalize; | ||
| font-size: 15px; | ||
| box-shadow: 0px 5px 12px 2px rgba(107, 69, 22, 0.88); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sporo właściwości z tego selektora pokrywa się z selektorem wyżej. Może powinniśmy jakoś skrócić zapis? Może wspólna klasa?
index.html
Outdated
| <a href="" class="link">portfolio</a> | ||
| <a href="" class="link">services</a> | ||
| <a href="" class="link">contact</a> | ||
| <nav class="navbar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Czy używamy tej klasy gdzieś w CSS? Jeżeli nie najlepiej ją usunąć, żeby zachować porządek w HTML. W innym przypadku w przyszłości kiedy będziemy musieli coś modyfikować możemy się trochę pogubić i nie będziemy wiedzieli czy możemy skasować jakąś klasę albo do czego ona tak naprawdę służy.
PS to nie jedyna klasa/id, która nie jest używana w CSS.
kontakt.html
Outdated
| <link rel="stylesheet" href="ourcontact.css"> | ||
| <link rel="stylesheet" href="contact-form.css"> | ||
| <link rel="stylesheet" href="footer.css"> | ||
| <link rel="stylesheet" href="reset.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O ile pliki CSS są dodane poprawnie o tyle złą praktyką jest ładowanie każdego pliku osobno. Może to nieznacznie wydłużyć czas wczytywania strony. Optymalizacja czyli przyspieszanie działania stron internetowych jest dziś dość modne. Proponowałbym więc stworzenie jednego pliku dla wszystkich styli strony i podzielenia go w jakiś czytelny sposób na sekcje (używając do tego komentarzy w CSS). Tutaj jest przykład jak można to mniej więcej podzielić:
https://coderwall.com/p/thuh8q/use-a-table-of-contents-in-your-css-file-for-an-overview
kontakt.html
Outdated
| </div> | ||
| <div id="top2"> | ||
| <a href="" class="color1">buy now</a> | ||
| <a href="" class="color1 text2">remowe firma</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bardzo dobre wykorzystanie generycznej klasy color1 w wielu miejscach 👍choć akurat w przypadku tego elementu ta klasa jest zbędna, bo w stylach dla #top2 a:last-child nadpisujemy biały kolor na #777777
nav.css
Outdated
| height: 60px; | ||
| line-height: 60px; | ||
| display: flex; | ||
| justify-content: space-between; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, czy mi się wydaje... czy powyższe cztery właściwości są dokładnie takie same jakie ma pasek nawigacji na samej górze? ;) jak mawia CSSowe porzekadło - nie powtarzajmy się w kodzie. Lepiej zamknąć to w generycznej klasie, tak jak zrobiliśmy to w przypadku color1 czy text2.
nav.css
Outdated
| line-height: 60px; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| margin-left: 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pytanie czy oby na pewno chcemy przesunąć w prawo całe menu? Jakby to menu nagle zmieniło kolor na czerwony - jakby to wyglądało? Oprócz tego, przesunięcie całego elementu wpływa na widok mobilny (negatywnie).
header.css
Outdated
|
|
||
| } | ||
| } | ||
| .linia1, .linia2, .linia3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selektor całkiem niezły, ale zastanówmy się czy jesteśmy w stanie zrobić to lepiej. Wszystkie trzy elementy to bezpośrednie i jedyne dzieci h1 w naszym głównym headerze. Skorzystałbym tutaj z bardzo fajnej właściwości jaką oferuje CSS czyli dziedziczenia. W dziedziczeniu oczywiście chodzi o to, że elementy podrzędne mogą przybrać właściwości swego rodzica. Oczywiście nie wszystkie właściwości, ale font-size i letter-spacing zdecydowanie należą do właściwości dziedziczonych.
A oto ich pełna lista: https://gist.github.com/dcneiner/1137601
W takim przypadku możemy skrócić ten selektor i usunąć niepotrzebne klasy w HTML.
Jeszcze innym rozwiązaniem jest też stworzenie generycznej klasy tak jak w przypadku color1 czy text2 i dodanie jej tym trzem elementom. W każdym razie zawsze powinniśmy dążyć i stosować rozwiązania, które tworzą jak najmniej kodu CSS i HTML.
main.css
Outdated
| } | ||
| main p { | ||
| font-size: 14px; | ||
| line-height: 2.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nie ma potrzeby dodawania .0 na końcu
index.html
Outdated
| <section class="card-section"> | ||
| <div class="card"> | ||
| <h5 class="color1 text2">i'm hangry desinger</h5> | ||
| <p class="color1">TheCodrops always hungry to lern.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, oba powyższe elementy pozycjonujemy niezależnie. Mowa o h5 i p. Obu nadajemy position: absolute i różne wartości dla left, bottom itp. Po pierwsze, możemy skrócić ten kod, a po drugie, ponieważ te elementy są ze sobą w pewien sposób powiązane, powinniśmy je złączyć w jedną grupę. Mam na myśli opakować je w jakieś opakowanie i pozycjonować tylko to opakowanie. Dzięki temu unikniemy problemu, że na telefonie czy tablecie te dwa elementy się jakoś rozjeżdżają.
kontakt.html
Outdated
| </main> | ||
| <div class="ourcontact"> | ||
| <div class="ourcontact1"> | ||
| <h1 class="text2">our contact</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Na stronie występuje zbyt dużo elementów h1. Na każdej jednej podstronie h1 może wystąpić tylko raz. Tag ten powinien określać najważniejszy nagłówek.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dziękuję za wszystkie komentarze, podpowiedzi i uwagi. Są naprawdę bardzo, bardzo pomocne.
Własnie zaczęłam już poprawiać stronę, mam nadzieję na szybkie postępy i że będzie jeszcze ze mnie jakiś pożytek :)
No description provided.