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

Личный проект: завершаем базовую стилизацию #6

Open
wants to merge 12 commits into
base: master
from

Conversation

Vogrim21 added 2 commits Feb 12, 2020
исправления
!!!
@keksobot keksobot changed the title Базовая стилизация Личный проект: начинаем базовую стилизацию Feb 13, 2020
keksobot added a commit that referenced this pull request Feb 13, 2020
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Feb 13, 2020

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

@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Feb 13, 2020

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

keksobot added a commit that referenced this pull request Feb 13, 2020
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 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
catalog.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Vogrim21 added 5 commits Feb 19, 2020
!!!
Roboto
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 Show resolved Hide resolved
css/style.css Show resolved Hide resolved
fonts/shrift Outdated Show resolved Hide resolved
catalog.html Show resolved Hide resolved
catalog.html Show resolved Hide resolved
catalog.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@keksobot keksobot changed the title Личный проект: начинаем базовую стилизацию Личный проект: завершаем базовую стилизацию Feb 23, 2020
@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Feb 23, 2020

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

keksobot added a commit that referenced this pull request Feb 23, 2020
Vogrim21 added 5 commits Feb 23, 2020
delete
delete
delete
delete
Добавление изображений
Добавление новых
Copy link
Collaborator

efiand left a comment

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

catalog.html Show resolved Hide resolved
@@ -89,76 +89,76 @@ <h1 class="promo">Магазин готовых шаблонов</h1>
</li>
</ul>
</fieldset>
<button type="submit">Показать</button>
<button class=" btn filter-btn" type="submit">Показать</button>

This comment has been minimized.

Copy link
@efiand

efiand Feb 25, 2020

Collaborator

Пробел после кавычки

This comment has been minimized.

Copy link
@efiand

efiand Feb 25, 2020

Collaborator

Чисто рекомендация: тут кнопки (сабмит фильров и пагинация) по количеству отличий вполне тянут на другой класс, не .btn.

<ul class="sorting">
<li><a class="sorting-item" href="#nowhere">По цене</a></li>
<li><a class="sorting-item sorting-item-1" href="#nowhere">По цене</a></li>

This comment has been minimized.

Copy link
@efiand

efiand Feb 25, 2020

Collaborator

Нездоровая тенденция везде лепить -1. Это уместно для чего-то совсем уж серийного типа слайдов, и то в качестве схоластического обхода допустимого в реальности исключения инлайнить background-image. Тут важно не то, что ссылка первая, а то что на первой ссылке дизайнер захотел показать выбранный фильтр. Мог на второй или третьей, но вот приспичило на первой, что ж поделать.

<p class="products-txt">Промо-сайт производителя крафтового кваса</p>
<a class="btn products-btn" href="#nowhere">9900</a>
</li>
</ul>
<ul class="pagination-list">
<li>
<a class="pagination-item" href="#nowhere">1</a>
<a class="btn pagination-item" href="#nowhere">1</a>

This comment has been minimized.

Copy link
@efiand

efiand Feb 25, 2020

Collaborator

Никакого приема, чтобы показать текущую ссылку

  • доп. класс
  • стилизация через :not([href]) (убрав href из разметки)
    не использовано.
</li>
<li>
<a class="pagination-item" href="#nowhere">Следующая</a>
<a class="btn pagination-item-next" href="#nowhere">Следующая</a>

This comment has been minimized.

Copy link
@efiand

efiand Feb 25, 2020

Collaborator

А что, она резко перестала быть при этом в том числе и pagination-item? те же стили по сути, я даже не знаю, нужно ли это -next, все может решить минимальная ширина (ведь цифры все же разные по ширине)

Copy link
Collaborator

efiand left a comment

Вторая итерация, продолжу позже

height: 146px;
.benefits-item {
min-height: 146px;
background-position: 100% 100%;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Если не делать псевдоэлемент, то фон висит сверху, то есть вторая координата 0. Лучше обезопаситься от слишком большой картинки:

  background-position: 50% 0;
  background-size: auto 146px;
width: 300px;
height: 146px;
.benefits-item {
min-height: 146px;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

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

@@ -206,25 +212,25 @@ body {
}

.page-list-item::before {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Это стоит воспринимать как иконку, хоть и рисованную на css, и позионировать абсолютно. Сейчас у нас она в инлайновом потоке и сильно зависит от ширины пробела.

height: 2px;
left: -5px;
content: "";
border-top: 2px solid #fb565a;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Если задана высота в 2px, зачем граница? Достаточно этот узкий прямоугольник фоном залить.

@@ -288,12 +302,15 @@ opacity: 0.3;
width: 2px;
background-color: red;
}

.popup-close::before {
transform: rotate(45deg);

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Хотел не нагружать тебя типовыми замечаниями, думал, везде поправишь отступы, собственно задача такая: везде поправить. Обычно так и делается: если проблема выявляется, то решается на уровне всего кода разработчика в этом проекте.

@keksobot

This comment has been minimized.

Copy link
Contributor

keksobot commented Feb 26, 2020

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

keksobot added a commit that referenced this pull request Feb 26, 2020

.range-min {
top: 26px;
left: 10px;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

При текущей реализации тут должен быть 0

@@ -381,30 +443,27 @@ transform: rotate(-45deg);
text-transform: none;
}

.filter-group-item .filter-check-label {
.filter-group-item {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Это были свойства элемента .filter-check-label: и блочность, и левый паддинг, зачем ты перенес их на родителя?

.filter-input-radio+ .filter-check-label:focus {
color: #000;
.filter-input-radio + .filter-check-label:hover,
.filter-input-radio + .filter-check-label:focus {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

С ховером это так и должно работать, но фокус ловит именно инпут

color: #ededed;
}

.filter-input-radio:disabled+ .filter-check-label::before {
.filter-input-radio:disabled + .filter-check-label::before {
border: 4px solid #ededed;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

достаточно менять border-color, а не перестраивать границу

position: relative;
.filter-input-check + .filter-check-label {
background-image: url("../img/check-off.svg");
background-repeat: no-repeat;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Всё кроме background-image можно описать просто как .filter-check-label и не дублировать


.filter-input-check + .filter-check-label:hover,
.filter-input-check + .filter-check-label:focus {
color: #000000;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Цвет и так черный, он потому и был серый, что opacity было 0.4, менять его не надо


.filter-input-check:checked + .filter-check-label {
background-image: url("../img/check-on.svg");
background-repeat: no-repeat;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

А что, внезапно режим повторения фона меняется? если нет, то зачем тут дублировать?

}

.filter-btn {
background-color: #ffffff;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Тебя обманули: (:
изображение
Она серая. И при этом такая же, как ссылки пагинации, логично видеть многие стили (кроме min-width) на их общем классе

box-shadow: inset 0px 3px 0 0 rgba(0, 1, 1, 0.1);
}

.sorting-title {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Было бы странно видеть смену дизайна (типографика) чего-то одного из этих сущностей без смены остальных:
изображение

Но так как это другой блок, не так критично, вполне жизнеспособно и такое прочтение

Copy link
Collaborator

efiand left a comment

Теперь точно всё!

text-decoration: none;
}

.sorting-item {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Этот селектор вешается на более старший элемент, чем .sorting a, рекомендую учитывать порядок, проще потом ориентироваться

line-height: 18px;
}

.sorting-item-1 {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

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

}

.sorting-dir-asc {
border-width: 10px 5.5px 0;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Браузеры по-разному округляют, пиши целые пиксели, по PP подбери нужный вариант.

}

.sorting-dir-desc {
margin: 0;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

А что, маргины были?

opacity: 0.2;
}

.sorting-dir-asc:hover,

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

На самом деле opacity: 1 - это показатель того, что сейчас отсортировано именно по этому направлению. Поэтому ширины границ - это действительно asc/desc, а вот opacity - это на другом классе, и тут у меня предложение стилизовать непрозрачность для обоих типов сортировки на уровне всего списка и создать универсальный класс "текущий признак сортировки" (независимо от списка), который просто делает opacity: 1.

color: #000000;
}

.products-title {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Сначала описан потомок, потом только родитель, лучше наоборот


.products-title a:focus,
.products-title a:hover {
color: #fb565a;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

На самом деле у нас такие же взаимоотношения имеются в меню, и мне видится такой вариант просто дефолтным для ссылок в этом проекте,

a:hover,
a:focus {}

но не настаиваю

}

.products-btn:active {
box-shadow: inset 0px 3px 0 0 rgba(0, 1, 1, 0.1);

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Да, с цветом там конечно есть баг макета, приходится под него подстраиваться согласно ТЗ, но тень-то как у .btn (печально, что у тебя devtools еще вызывает отторжение, без него выше джуна не взлететь):
изображение

И, кстати, про 0px речь уже шла, 0px введи глобальным поиском в редакторе (но с пробелом перед нулем, а то найдет кучу ненужного) и исправь, глазами оно конечно неблагодарно искать

}

.filter-input-check:checked+ .filter-check-label::after {
background-image: url('../img/сheсkbox-on.svg');
.pagination-item {

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Не называй ссылки -item, реально неудобно.
-item это <li> (если ему нужен класс). Если в проекте одни дивы и ссылки всегда в общем родителе, то канает, а так у тебя -item это элементы списков, и сохраняй эту преемственность, ссылки это вполне -link, просто и понятно, не надо даже комментарий к коду писать

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

И тут куча стилей перекликается с сабмитом формы, я бы задумался над тем, что это .inner-control (всплыли на внутренней странице) или .catalog-control (в каталоге): без наличия всех макетов можем только гадать. -control довольно распространено ("контрол" значит интерактивный элемент, без привязки к тегу). Но это только как пример названия и вообще унифицировать стили это только рекомендация.

background-color: #eeeeee;
color: #000000;
line-height: 18px;
width: 260px;

This comment has been minimized.

Copy link
@efiand

efiand Feb 26, 2020

Collaborator

Вот это даже без второго класса проканает, ведь совсем как кнопка сабмита, а у тех ссылок класс pagination-link может отвечать за другую min-width просто и за правый маргин (а этот контрол последний, ему в общем и не нужен правый маргин)

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.