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

Стихин Семен #77

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Стихин Семен #77

wants to merge 17 commits into from

Conversation

samstikhin
Copy link

@samstikhin samstikhin commented Oct 24, 2017

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍏 Пройден линтинг и базовые тесты

@samstikhin
Copy link
Author

Знаю, что картинок нет, они пока сильно плывут при изменении окна.

@VasiliiKuznecov
Copy link

Привет! Твой ментор пока в отпуске, поэтому сейчас переназначу на другого
В следующий раз можешь попросить, чтобы тебе назначили обратно старого)

@VasiliiKuznecov
Copy link

🔔

Copy link

@trixartem trixartem left a comment

Choose a reason for hiding this comment

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

Замечания:

  • Нет цитат
  • Маловато картинок с подписями
  • Первая буква не выделена
  • посмотри задание внимательно
    🍅

index.css Outdated
@font-face
{
font-family: machina;
src: url('fonts/machina.ttf');

Choose a reason for hiding this comment

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

Не самый лучший формат

index.css Outdated

header
{
font: 70pt machina;

Choose a reason for hiding this comment

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

pt? Почему? так же не хвататет фолбэка для шрифта

main
{
margin-left: 60px;
font: 1em zelek;

Choose a reason for hiding this comment

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

Где-то em где-то pt, приведи к одному виду

index.css Outdated

h1
{
margin-left: 2%;

Choose a reason for hiding this comment

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

От чего считается процент? точно он здесь нужен?

.block2
{
font-size: 1em;
border: 7px dotted #000;

Choose a reason for hiding this comment

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

Если делаешь разный бордер, то сделай что бы на стыке блоков был только один бордер, а не как сейчас 2(не очень красиво)

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍅 Не пройден линтинг или базовые тесты

@honest-hrundel
Copy link

🍏 Пройден линтинг и базовые тесты

Copy link

@trixartem trixartem left a comment

Choose a reason for hiding this comment

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

Есть пара комментариев 🍅

border: 2px dashed #000;
}

.block2

Choose a reason for hiding this comment

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

Название класса должно быть говорящим


.c2:checked ~ main
{
font-family: Arial;

Choose a reason for hiding this comment

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

Нужен фолбэк для шрифта

@trixartem
Copy link

@samstikhin Сделай, пожалуйста, правки

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

Successfully merging this pull request may close these issues.

5 participants