Skip to content
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

Личный проект: начинаем строить сетку на флексах #8

Merged
merged 12 commits into from Mar 13, 2020

Conversation

@keksobot keksobot changed the title Базовая сетка Личный проект: начинаем строить сетку на флексах Mar 2, 2020
Copy link
Collaborator

efiand left a comment

Пока только 2 замечаний и одна рекомендация.

catalog.html Outdated Show resolved Hide resolved
catalog.html Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
KaiZen-prog added 2 commits Mar 5, 2020
Исправил замечания по сетке, начал приводить значения line-height к макетным и вравнивать элементы через паддинги (см. хэдер сайта)
keksobot added a commit that referenced this pull request Mar 6, 2020
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Mar 6, 2020

♻️ Я собрал ваш пулреквест. Посмотреть можно здесь.

Copy link
Collaborator

efiand left a comment

Учитывая незаконченность обязательных в задании блоков, например футера, предположу, что задание еще в стадии разработки и не буду его комментировать.

Также не буду комментировать и однотипные проблемы, выявите их самостоятельно:

  • большие кастомные паддинги там, где явная центровка текста (телефон в хедере и адрес под ним же, например)
  • разные паддинги, например у среднего блока с телефоном и адресом в шапке, зачем? Логично одинаковые
  • верстка по макету "в лоб" без возможности дать немного переполнения: в той же средней шапке можно обойтись без маргинов, дав паддинги (чтобы совсем не было касания в край) и расположив правые элементы по правому краю
  • вообще не использовать больше паддингов чем надо, например в блоке телефон/адрес в шапке нижний паддинг можно задать родителю, в общем так и принято: обрамляющие паддинги - у родителя, чтобы при замене контента никогда не было касания в края.
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
img/icon-cart.svg Outdated Show resolved Hide resolved
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Mar 10, 2020

♻️ Я собрал ваш пулреквест. Посмотреть можно здесь.

keksobot added a commit that referenced this pull request Mar 10, 2020
catalog.html Outdated Show resolved Hide resolved
catalog.html Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Show resolved Hide resolved
catalog.html Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
Copy link
Collaborator

efiand left a comment

Построчно оставлены замечания к коду, но проблемы реализации проще показать наглядно, поэтому я просто оставлю это здесь (их мало, и это радует):

изображение

изображение

изображение
(потому что неверное деление на списки: три категории товаров и 2 преимущества)

изображение
(надо запретить кнопке flex-shrink)

изображение
У текстовых ссылок, не имеющих своего фона, фон родителя кликабельным быть не должен. Правильнее всего было бы ссылку положить в заголовок, нежели наоборот, ну раз так вам кажется семантически вернее, тогда пользуйтесь либо стандартными решениями (паддинги родителя вместо задания отступов каждому элементу, ведь о каждом потенциально добавленном элементе теперь думать, чтобы в края не прилип, а картинку можно и отрицательными маргинами вытолкать) - либо маргинами на ссылку вместо паддингов

изображение
При переполнении верхнего меню нижнее поехало влево. А ведь они одинаковым образом липнут к правому краю центровщика (flex-end by design)
а то пока такое неединообразие:
изображение

Рекомендации (думаю, на защите проскочит):
сделать тень на css, в крайнем случае позиционировать так, чтобы растягивалась
изображение
Как вариант:
изображение

и (не в упрек, но на будущее) обратите внимание, сколько вы надублировали стилей карточке товара и партнера, я про тени и рамки, вместо того чтобы дать им еще общий класс

css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Mar 13, 2020

♻️ Я собрал ваш пулреквест. Посмотреть можно здесь.

keksobot added a commit that referenced this pull request Mar 13, 2020
Copy link
Collaborator

efiand left a comment

Молодей, проделана большая работа, но некоторые вопросы остаются

изображение

Тут это какой-то костыль, значит что-то сделано не так, можно подогнать под макет и иначе, задав ширины и левый автомаргин правой колонке. Непонятно, зачем абсолют, если левую колонку можно сделать флекс-элементом, и не придется городить ни минимальной высоты, ни костыля типа height: 100% на правую колонку.

Не покидает ощущение, что верт. паддинги .button все-таки везде одинаковые должны быть- 11 и 9, странно, что одна и та же сущность так по-разному делается.
Возможно, где-то просто оставлена инлайновость ссылок и зависит от line-height анонимного бокса, сделайте .button по умолчанию, например, инлайн-блочной, тем более что тег кнопки такой и есть по умолчанию

css/style.css Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
css/style.css Outdated Show resolved Hide resolved
@efiand

This comment has been minimized.

Copy link
Collaborator

efiand commented Mar 13, 2020

изображение

Не задан дефолтный цвет фона, могу предложить задавать его для этого блока через 3n, 3n + 1 и 3n + 2, а для категорий - по четным/нечетным.
А что касается иконок - они же такие не потому, что блок второй или третий, а от контента сильно зависят, значит тут класс нужен (по уму style="background-image()", но академия запрещает, чтобы лишних свойств так не писали, это пожалуй единственное исключение и есть

@KaiZen-prog

This comment has been minimized.

Copy link
Contributor Author

KaiZen-prog commented Mar 13, 2020

Насчет несимметричного блока с сервисами - фоновые изображения на двух слайдах выходят за правую границу общей колонки сайта в 940px, отсюда сдвиг блока

@efiand

This comment has been minimized.

Copy link
Collaborator

efiand commented Mar 13, 2020

Насчет несимметричного блока с сервисами - фоновые изображения на двух слайдах выходят за пределы общей колонки сайта в 940px, поэтому я сдвинул его содержимое вправо

Действительно. Ну ладно. На лекции перестали учить, как обрезать лишнее, раньше показывали image -> trim или canvas size.

@efiand

This comment has been minimized.

Copy link
Collaborator

efiand commented Mar 13, 2020

Вот что еще важно
изображение

Если окно скукожить, а потом растянуть, будет такое, если не задать body минимальную ширину, в нашем случае 960 - на ней не должна появляться горизонтальная прокрутка вообще, так что может быть картинки лучше подправить, или повесить на body еще overflow-x: hidden.

Поэтому, кстати, раз минимальная ширина все равно есть, контейнеру паддинги не особо и нужны, только лишние усложнения

Подкорректировал недочеты

За выходные постараюсь сверстать всё что осталось
Copy link
Collaborator

efiand left a comment

Критические ошибки в рамках задания устранены.
Остальное можно (а что-то и нужно) исправить со следующим заданием

@@ -51,6 +51,9 @@

<div class="user-lower-panel">
<a href="#nowhere">Мои заказы</a>
<svg width="6" height="6">

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Прикольно )
Но псевдоэлементом .user-lower-panel li + li::before куда оптимальнее (ну да, соседствующим ссылкам мало что добавить можно, а тут и обертка, и никто не придерется, сами себя заперли в решении). Да и при переполнении надо еще про эту шняжку векторную думать. Кстати, а зачем так сложно - и обводка, и заливка, почему одной заливкой было не обойтись?

@@ -183,7 +188,7 @@ a {
background-color: #293449;
}

.header-menu-button:before {
.header-menu-button::before {

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

В паре мест :after осталось поправить

padding-left: 17px;
}

.user-lower-panel svg {
position: absolute;

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Очень шаткое решение, при увеличении ширины текста соседних ссылок и при переполнении на другую строку оно сломается.


display: flex;
flex-wrap: wrap;
justify-content: space-between;

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Сочетание этого с многострочностью таит проблемы, когда второй ряд неполон, автомаргины надежнее в этом плане

.product-category:nth-child(3n+2) {
background-color: #3bbce3;
}

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Поскольку имеющиеся ниже цвета на конкретных карточках удовлетворяют имеющимся вариантам (если нет - просто надо подобрать соответственно) - то их нет смысла дублировать

.service-category:nth-child(2n) {
background-color: #ffc047;
}

.service-category:nth-child(1) {
background-color: #8ed78f;

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Применится 762 строка
изображение
не было смысла оставлять

.services-slider-slide-3 p {
width: 300px;
.services-slider-slide-1 p:first-of-type {
padding-bottom:0;

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Пробел после двоеточия


min-width: 960px;

overflow-x: hidden

This comment has been minimized.

Copy link
@efiand

efiand Mar 13, 2020

Collaborator

Это я случайно посоветовал как для адаптивной верстки, у нас как раз при ширине менее 960 должна быть прокрутка. Вообще что-то я не сразу сообразил, что речь не о контентных, а о фонах, но ведь фоны же и так режутся!
Так что уберите вообще все костыли, включая те странные разные паддинги и другую ширину и просто подвиньте фон вправо))

@efiand efiand merged commit 086634e into htmlacademy-htmlcss:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.