Skip to content

CSS homework - #3

Open
Georgiy0 wants to merge 4 commits into
masterfrom
CSS
Open

CSS homework#3
Georgiy0 wants to merge 4 commits into
masterfrom
CSS

Conversation

@Georgiy0

@Georgiy0 Georgiy0 commented Mar 25, 2017

Copy link
Copy Markdown
Collaborator

CSS homework
CSS привел в соответствие с образцом. Подключил sass.
Добавил ветку для GitHub pages: https://ok-technopolis.github.io/Kubrin-Georgiy/

@Georgiy0
Georgiy0 requested a review from DmitryMakhnev March 25, 2017 18:58

@DmitryMakhnev DmitryMakhnev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Подружить с #2

@DmitryMakhnev DmitryMakhnev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Изучить разбор дз.
Перевести на scss

Comment thread index.html Outdated

<div class="todo-list">
<div class="todo-item">
<textarea class="todo-item_text" aria-label="Ввести текст">HTML</textarea>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

будут проблемы с фаллбеком на флоатах

Comment thread index.html Outdated
<div class="todo-actions-bar">
<div class="todo-actions-bar_counter" aria-label="Счетчик невыполненых">3 items left</div>

<a class="todo-filter __active" href="#" data-filter="all" aria-label="Выбрать все">All</a>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Переделать на button

}

.__sublayer1 {
width: 556px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ширина не должна быть захардкожена см разбор дз


.todos-title {
font-family: "Arial" serif;
font-size: 60pt;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Используем px

}

.todos-title {
font-family: "Arial" serif;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему первым не стоит шрифт из макета?

align-self: center; flex: 1 3; order: 2;
}

.todo-item_checkbox {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

align-self: center; order: 1;
}

.todo-item_checkbox_target {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

чекбокс надо сделать как независимый компонент, смотри разбор дз

border: 1px solid #E6E6E6;
}

.__checked {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

.todo-item_checkbox_target:checked > .todo-item_checkbox_visual {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@DmitryMakhnev DmitryMakhnev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поправить замечания

Comment thread package.json Outdated
},
"repository": {
"type": "git",
"url": "git@github.com:ok-technopolis/preprocessor-stub.git"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У вас вроде бы другой проект

Comment thread public/index.html
<div class="todo-body">

<div class="todo-add">
<div class="action todo-add_select-all-action">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Этот тег не закрыт (

Comment thread public/index.html
</div>
</div>

<div class="todo-item">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не вижу кейса с модификатором отмеченности айтема

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправил, добавил модификатор __checked.

Comment thread src/styles/base/action/action.scss Outdated
position: absolute;
@include absolute-full();
}
button.action_target, input.action_target {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправил

Comment thread src/styles/base/title/title.scss Outdated
margin: $offset_2x;
padding: $offset;
// test
background: url('../../../images/icons/favorites-off.png');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

наверное стоит удалить

transition: opacity .2s;
}
.todo-add_select-all-action {
visibility: visible;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем visibility: visible;?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тестировал, забыл убрать. Убрал.

Comment thread public/index.html Outdated
</div>

</div>
<div class="todo-body __sublayer1"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Декоративный элемент после боди !== body

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправил, реализовал на псевдоэлементах :after и :before.

z-index: 3;
}

.__sublayer1 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Нельзя обращаться по модификатору без класса блока или элемента, т.к. представьте сколько блоков могут иметь модификаторы __active, __ready и т.д.
  2. декрация .todo-body !== .todo-body, посмотрите как это сделано в разборе д.з.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправил, реализовал декорацию todo-body на псевдоэлементах как в разборе.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Посему тогда остались стили для todo-sublayer ?

margin: $offset_2x 0;
}

.todos-actions-bar .todos-filters {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправил

@DmitryMakhnev DmitryMakhnev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В целом ок, но замечания надо поправить

Comment thread public/index.html

<div class="todo-list">
<div class="todo-item">
<div class="input-checkbox">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нужно сделать чекбокс конкретным элементом todo-item, т.к. в противном случае, при добавлении второго чекбокса, будут проблемы

Comment thread public/index.html
</div>

<div class="todo-item">
<div class="input-checkbox __checked">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Чекбоксу модификатор не нужен, модификатор готовности должен быть у todo-item, т.к. от этого может зависеть очень многое внутри компонента

transition: opacity .2s;
}
.todo-add_select-all-action {
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

видимо куда-то потерялось скрытие

z-index: 3;
}

.__sublayer1 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Посему тогда остались стили для todo-sublayer ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants