Conversation
devmentor-pl
left a comment
devmentor-pl
left a comment
There was a problem hiding this comment.
Adamie,
Kierunek jest ok 👍
Jest jednak parę ważnych rzeczy do poprawy - postaraj się je wdrożyć :)
| padding-right: 75px; | ||
| margin-left: auto; | ||
| margin-right: auto; | ||
| max-width: 1150px; |
There was a problem hiding this comment.
Maksymalną rozdzielczość ustawiamy dla zawartości, ale nie tła. Tło powinno być nadane elementowi o 100% szerokości, aby nie wyglądało to na ucięte - tak jak obecnie.
Zobacz jak to u mnie wygląda:
PS. Zobacz jeszcze komentrz do pliku CSS
index.html
Outdated
| </head> | ||
| <body> | ||
| <!--section first with logo menu and first content--> | ||
| <header class="header container"> |
There was a problem hiding this comment.
Aby rozwiązać problem, o którym piszę przy pliku CSS wystarczy zrobić coś takiego:
<header class="header">
<div class="container">
Wtedy tło będzie na 100%, a zawartość będzie ograniczona.
styles/base.css
Outdated
| } | ||
| .header { | ||
| background: radial-gradient(rgb(194, 74, 126), rgb(105, 86, 235)); | ||
| height: 364px; |
There was a problem hiding this comment.
Lepszym rozwiązaniem jest pozwolenie, aby zawartość wypełniła element i ona określała jej wysokość - nigdy nie wiemy czy nie będzie ona większa lub mniejsza i wtedy będzie źle wyglądałą wysokość na sztywno. Jeśli potrzebujesz odstępów to robisz je przy pomocy np. padding.
styles/base.css
Outdated
| } | ||
| .collaborate__container { | ||
| background-color: rgb(249, 250, 250); | ||
| height: 45px; |
There was a problem hiding this comment.
W większości przypadków ta wysokość jest raczej nie potrzebna.
index.html
Outdated
| Pick any of our super affordable pricing plans | ||
| </h2> | ||
| </div> | ||
| <div class="price__list block"> |
There was a problem hiding this comment.
Dość małe to wszystko wygląda:
Musisz wiać pod uwagę, że strona będzie oglądana na różnych rozdzielczościach. Jeśli masz mały ekran w laptopie to powinieneś obejrzeć projekt na innym monitorze i ocenić czy jest ok. Musimy pamiętać, że nie tylko my przeglądamy stronę, którą wykonujemy :)
index.html
Outdated
| </h2> | ||
| </div> | ||
| <div class="price__list block"> | ||
| <div class="list__first"> |
There was a problem hiding this comment.
Zobacz sama nazwa klasy wzkzuje, żę to lista to może każdy element to li zamiast div, a ich rodzicem zrobić ul zamiast div? PS. Tak można mieć listę w liście ;)
index.html
Outdated
| <h4 class="list__offer bold"> | ||
| Up to 7 Projects 2 Additional Developers | ||
| </h4> |
There was a problem hiding this comment.
Ten element to lista elementów wchodzących w skład pakietu więc zrobiłbym:
<ul>
<li>Up to 7 Projects</li>
<li>2 Additional Developers</li>
...
</ul>
index.html
Outdated
| <h4 class="list__offer bold"> | ||
| Up to 7 Projects 2 Additional Developers | ||
| </h4> |
There was a problem hiding this comment.
Ułatwiłeś sobie trochę, bo ta lista w każdym pakiecie jest dłuższa ;P
index.html
Outdated
| <h3 class="member__description light"> | ||
| The one that puts it all together | ||
| </h3> | ||
| <h4 class="member__contact"> |
There was a problem hiding this comment.
Czy faktycznie aż tyle tych nagłówków jest tutaj potrzebnych? Zdecydowanie zrezygnowałbym z h4, może nawet h3 na rzecz odpowiednio div oraz p
index.html
Outdated
| <div class="specifications__table--background"> | ||
| <img | ||
| src="/images/responsive.svg" | ||
| class="specifications__table--img" |
There was a problem hiding this comment.
Jesli używamy modyfikatora z BEM to powinien on występować razme z Elementem czyli tutaj:
specifications__table specifications__table--img. Więcej znajdziesz tutaj: https://devmentor.pl/b/metodologia-bem-w-css-i-sass
devmentor-pl
left a comment
devmentor-pl
left a comment
There was a problem hiding this comment.
Adamie,
Zdecydowanie lepiej! Gratuluję!
Zostawiłem parę drobnych uwag :)
| <img class="ads__img" src="images/screen.png" /> | ||
| </aside> | ||
| <header class="header"> | ||
| <div class="header_container container"> |
There was a problem hiding this comment.
Tutaj raczej 2x _ czyli header__container. Jeśli chcesz mieć nazwę z 2 słów to użyj - tj. header-container
| Beautiful Free Nova <br /> | ||
| template. | ||
| </h1> | ||
| <h2 class="article__text regular"> |
There was a problem hiding this comment.
Tutaj raczej użyłbym samego p- ale nie jest to błąd ;)
| A top notch premium quality<br /> | ||
| template for your next web <br />project. | ||
| </h2> | ||
| <h3> |
There was a problem hiding this comment.
Natomiast tutaj nie używałbym już h3 - to raczej normalny button, nie trzeba go "zaznaczać" przez nagłówek.
| <div class="price__list--text regular"> | ||
| Up to 25 Projects 2 Additional Develepers Unlimited Support | ||
| </div> |
There was a problem hiding this comment.
Tutaj raczej zrobiłbym ul > li bo to lista elementów pakietu cenowego
| margin-left: auto; | ||
| margin-right: auto; | ||
| max-width: 1150px; | ||
| padding: 10px 20px; |
There was a problem hiding this comment.
Tutaj jeszcze zabrakło margin: 0 auto, aby wyśrodkować zawartość. Na mniejszym ekranie tego nie widać, ale jak masz większy to lepiej, aby odstępy pojawiły się po bokach niż całość była przykjelona do lewego boku.
2 participants
zrobiony projekt :)