add static web page based on Webscope project#187
add static web page based on Webscope project#187MagdalenaKoltuniak wants to merge 2 commits intodevmentor-pl:masterfrom
Conversation
devmentor-pl
left a comment
devmentor-pl
left a comment
There was a problem hiding this comment.
Magdo,
Projekt prezentuje się bardzo dobrze! 👍
Zostawiłem parę mniej istotnych uwag w komentarzach, które głównie skupiają się na semantyce ;)
styles/index.css
Outdated
| height: 100px; | ||
| } | ||
|
|
||
| .header__logo img { |
There was a problem hiding this comment.
Jeśli korzystamy z BEM to nawet taki img powinien mieć klasę :)
index.html
Outdated
| <nav class="header__menu"> | ||
| <ul class="header__menu-list"> | ||
| <li><a class="header__menu-link" href="">The Team</a></li> | ||
| <li><a class="header__menu-link" href="">Pricing</a></li> | ||
| <li><a class="header__menu-link" href="">Features</a></li> | ||
| </ul> | ||
| </nav> |
There was a problem hiding this comment.
Sporo tych "Elementów" w Bloku header. Spokojnie można dodać kolejne Bloki tj. np.
<nav class="header__menu menu"><!-- nowy blok o nazwie "menu"
<ul class="menu__list"><!-- nowy element tego bloku o nazwie "list"
Mniejsze grupy (bloki) będą czytelniejsze.
TO co zrobiłaś to nie jest błąd, ale można trochę usprawnić :)
index.html
Outdated
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <link rel="preconnect" href="https://fonts.googleapis.com"> | ||
| <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> | ||
| <link href="https://fonts.googleapis.com/css2?family=Montserrat:wght@100..900&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
Pytanie czy faktycznie potrzebujesz rozmiarów od 100 do 900 - może nie korzystasz ze wszystkich? Warto ograniczyć do wykorzystywanych, aby obniżyć ilośc pobieranych KB.
index.html
Outdated
| </header> | ||
| <main> | ||
| <section class="features container"> | ||
| <div class="features__header box"> |
There was a problem hiding this comment.
Spokojnie można użyć znacznika header - on nie jest wykorzystywany jedynie w nagłówku strony, ale może też być nagłówkiem dla sekcji lub artykułu.
index.html
Outdated
| </ul> | ||
| </nav> | ||
| </div> | ||
| <div class="header__cta box"> |
There was a problem hiding this comment.
Wrzuciłbym znacznik article, bo to jedna spójna treść, która znajduje się wew.
index.html
Outdated
| </div> | ||
| </section> | ||
|
|
||
| <section class="logos container"> |
There was a problem hiding this comment.
Może nawet aside - bo te grafiki to coś mniej istotnego.
index.html
Outdated
| </section> | ||
|
|
||
| <section class="pricing container"> | ||
| <div class="pricing__header box"> |
index.html
Outdated
| <div class="pricing__options box"> | ||
| <div class="pricing-card"> |
There was a problem hiding this comment.
Tutaj jest lista cen więc ponownie bym zrobił ul > li
index.html
Outdated
| </section> | ||
|
|
||
| <section class="team container"> | ||
| <div class="team__header box"> |
index.html
Outdated
| <div class="team__members box"> | ||
| <div class="team-card"> |
There was a problem hiding this comment.
Lista członków zespołu więc proponuję ul > li
devmentor-pl
left a comment
devmentor-pl
left a comment
There was a problem hiding this comment.
Magdo,
Zmiany są jak najbardziej ok 👍 🥇
Zostawiłem jeszcze jeden komentarz dot. pobierania fontów.
| } | ||
|
|
||
| .header__logo img { | ||
| .header__logo .header__image { |
There was a problem hiding this comment.
Teraz raczej powinno już wystarczyć samo .header__image jeśli tak nie jest to należy użyć modyfikatora tj. np. .header__image--active
| } | ||
|
|
||
| .header__menu-list { | ||
| .menu__list { |
| <ul class="menu__list"> | ||
| <li><a class="menu__link" href="">The Team</a></li> | ||
| <li><a class="menu__link" href="">Pricing</a></li> | ||
| <li><a class="menu__link" href="">Features</a></li> |
| </nav> | ||
| </div> | ||
| <div class="header__cta box"> | ||
| <article class="header__cta box"> |
| </div> | ||
| <div class="feature-card"> | ||
| </li> | ||
| <li class="feature-card"> |
| <link rel="preconnect" href="https://fonts.googleapis.com"> | ||
| <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> | ||
| <link href="https://fonts.googleapis.com/css2?family=Montserrat:wght@100..900&display=swap" rel="stylesheet"> | ||
| <link href="https://fonts.googleapis.com/css2?family=Montserrat&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
W ten sposób pobieramy tylko 1 wersję fontu tj. domyślną (400). To rozwiązanie jest szybie, ale powoduje, że przeglądarka sama "pogrubia" wskazane elementy. To może spowodować, że na różnych przeglądarkach może to wyglądać inaczej. Najlepszym kompromisem jest wskazanie konkretnych fontów z jakich korzystasz tj. https://fonts.googleapis.com/css2?family=Montserrat:wght@400;700&display=swap
Jeśli potrzebujesz również wersji pochylonej to możesz zrobić to tak:
<link href="https://fonts.googleapis.com/css2?family=Montserrat:ital,wght@0,400;0,700;1,400;1,700&display=swap" rel="stylesheet">
| </div> | ||
| <div class="pricing-card"> | ||
| </li> | ||
| <li class="pricing-card"> |
| </div> | ||
| <div class="team-card"> | ||
| </li> | ||
| <li class="team-card"> |
2 participants
No description provided.