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

Начинаем верстать адаптивные сетки #9

Merged
merged 2 commits into from Dec 1, 2019

Conversation

@keksobot keksobot changed the title Добавляет начальную верстку адаптивных сеток Начинаем верстать адаптивные сетки Nov 29, 2019
keksobot added a commit that referenced this pull request Nov 29, 2019
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Nov 29, 2019

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

@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Nov 29, 2019

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

keksobot added a commit that referenced this pull request Nov 29, 2019
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Nov 29, 2019

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

keksobot added a commit that referenced this pull request Nov 29, 2019
Copy link
Collaborator

efiand left a comment

Адаптив методом mobile-first понят правильно, существенных замечаний нет.
Ревью содержит в основном советы, как сделать лучше (на мой взгляд).

background-image:
url("../img/icon-editor-diagram-fill.svg"),
url("../img/icon-timer.svg");
background-repeat: no-repeat, no-repeat;

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

(На заметку) Когда вы разделяете свойство background на составные и repeat везде одинаковый, можно писать без дублирования.

@@ -7,6 +7,16 @@
padding: 0 37px 0;

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Немного проворонил в тот раз, пожелание: когда верхнее и нижнее свойства одинаковые, писать двумя значениями вместо трех.

@media (min-width: 660px) {
padding: 38px 0 31px 310px;

&::before {

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Пожелание: использовать брейкпоинты только для модификации, то есть почти все свойства писать в исходном правиле, а для планшета только content.
на мой вхзгляд, так удобнее в перспективе поддержки проекта.

padding: 38px 0 31px 310px;

&::before {
content: "24";

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

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

@@ -7,6 +7,16 @@
padding: 0 37px 0;

list-style: none;

@media (min-width: 660px) {

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Много раз используются одни и те же брейкпоинты.
Пожелание: использование для этой цели переменных. В variables.less можно написать

@tablet: ~"(min-width: 660px)";

а в блоках использовать @media @tablet {}.
Это не просто сокращает код, повышая читаемость (660 и 960 надо останавливать внимание, чтобы отличить) и скорость написания, но и позволяет решать нередкую задачу: изменение брейкпоинтов в ходе существования проекта. Еесли при этом где можно используется резиновость, то есть фиксируется (если фиксируется вообще) максимально старший блок (а внутри все резиновое) - процесс вместо дня работы занимает 5 минут, буквально на той неделе сталкивался, дизайнеры ошиблись).
Да, контекстная замена рулит, но только если брейкпоинты простые, а это не всегда так. В общем, я на стороне переменных для этой цели.


display: flex;
flex-direction: column;
flex-grow: 1;

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Главное периодически проверять в IE 11, который нам по ТЗ приходится поддерживать.
Альтернативный вариант прибить содержимое книзу - непосредственно нижнему блоку задать margin-top: auto.
В целом смущает применение свойств флекс-элемента(flex-grow) при отсутствии свойств флекс-контейнера на .images. Да, это проходит критерии Академии (если флекс-контейнер на другом БЭМ-блоке), но это нарушение БЭМ-методологии, блоки независимы.
С другой стороны, зачем вообще тут флексовое решение, если и так решается через min-height?
Возможно, я не до конца понял перспективы этой реализации.

padding-right: 30px;
}

&::before {

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

У модификаторов могут быть свои псевдоэлементы, у псевдоклассов - тоже, а у модификаторов при этом свои псевдоклассы, поэтому советую порядок псевдоэлементы -> вложенные теги и классы (то есть 2 группы сущностей, которые вложены, при этом у первой группы - псевдоэлементов - вложенности уже никакой, а вложенность второй группы на вложенность первой уже никак не влияет) -> псевдоклассы (в которых может быть переопределение как псевдоэлементов, так и вложенных тегов) -> модификаторы (в которых может быть как переопределение вложенных структур само по себе, так и за псевдоклассами, причем и то и другое может иметь место, тогда внутренний порядок по тем же принципам).
Что касается медиазапросов (на каждый элемент или на элемент только нулевого уровня вложенности) - тут не так категорично, я сам изначально писал как у вас сейчас, но на большинстве реальных проектов портянка в брейкпоинте все равно такая вырастает, что экрана на два прыгать приходится, именно потому и пришел к этому: https://github.com/efiand/efiand.github.io/blob/master/docs/css-rules-order.md

@@ -37,6 +94,13 @@

text-align: center;
text-transform: uppercase;

@media (min-width: 960px) {

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Линтер пропустил пустую строку сразу после начала правила? это нехорошо, на мой взгляд - как в плане организации css-кода как такового, так и настроек стайллинта (в будущем советую обзавестись соотв. настройкой, у stylelint богатая документация).

@@ -4,39 +4,75 @@
align-items: center;
padding-top: 40px;
padding-bottom: 40px;

&--hidden {

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Пожелание: при наличии уже в проекте универсального способа (миксование класса .hidden) не умножать пути решения. Хелперы мы пишем в самом конце, и никакой проблемы с каскадом быть не должно.

Пожелание: для очевидности решения (что это адаптивное скрытие) - называть модификатор соответственно, то есть --mobile-hidden, а не просто --hidden, а то удаленного выше комментария бы и не случилось )

This comment has been minimized.

Copy link
@efiand

efiand Dec 1, 2019

Collaborator

Это, кстати, та ситуация, когда можно (не обязательно, но и не запрещено) использовать @mobile-only: ~"(max-width: 659px)";

@efiand efiand merged commit ae95ba5 into htmlacademy-adaptive:master Dec 1, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.