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

Мифтахов Валерий #75

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

Мифтахов Валерий #75

wants to merge 7 commits into from

Conversation

vmiftakhov
Copy link

@vmiftakhov vmiftakhov commented Oct 24, 2017

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

Copy link

@Mokoshka Mokoshka left a comment

Choose a reason for hiding this comment

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

  • Хочется, чтобы заголовок не переносился
    image
  • При наведении на label для выбора стиля хочется, чтобы курсор изменялся. Так будет понятно, что элемент интерактивный

index.css Outdated
column-rule: 1px solid #fff;
}

.labelButton
Copy link

Choose a reason for hiding this comment

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

Принято названия классов указывать через дефиз: label-button

index.css Outdated
font-family: Arial;
text-align: center;
column-count: 3;
padding: 50 0;
Copy link

Choose a reason for hiding this comment

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

50 чего? Браузер не понимает такого
image

index.css Outdated
.labelButton
{
line-height: 3em;
font-family: Arial;
Copy link

Choose a reason for hiding this comment

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

Стоит указать "запасным" шрифтом шрифт без засечек (sans-serif). Это нужно для того, чтобы в случае, если шрифта у пользователя нет, браузер подставил свой с указанным начертанием

index.html Outdated
<br>
<label for="black_and_white">Тема 1</label>
<label for="white_and_black">Тема 2</label>
<br>
Copy link

Choose a reason for hiding this comment

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

Давай сделаем этот блок без использования br

index.html Outdated
<h1>Newspaper - fish</h1>
<section>
<div class="picture">
<img src="picture.png" alt="picture">
Copy link

Choose a reason for hiding this comment

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

title не хватает у изображения

index.html Outdated
<div class="picture">
<img src="picture.png" alt="picture">
</div>
<article class="article2">
Copy link

Choose a reason for hiding this comment

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

Кажется, этот класс нигде не используется

Copy link
Author

Choose a reason for hiding this comment

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

Добрый день, спасибо, что проверили :-) . Правильно ли я Вас понимаю, классы должны присутствовать только там, где они используются? "Хочется, чтобы заголовок не переносился" - выполнил только для h1, так как у h2 длинные заголовки, если только скрывать лишнее, но тогда теряется смысловая информация.

Copy link

Choose a reason for hiding this comment

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

Зачем указывать класс, к которому ты никак не привязываешься? У него нет стилей в твоем css.
У колонок есть много интересных свойств. Например, одно из них break-inside. Попробуй, может поможет решить проблему с заголовками без скрытия лишнего

index.css Outdated
text-indent: 2em;
}

input[value='smoll']:checked ~ main
Copy link

Choose a reason for hiding this comment

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

Может быть все-таки small?

index.css Outdated

input[value='TrixieCyr-Plain']:checked ~ main
{
font-family: TrixieCyr-Plain;
Copy link

Choose a reason for hiding this comment

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

Лучше указывать названия шрифтов единым стилем, сейчас они разные. И указать дополнительные шрифты на случай, если эти по каким-либо причинам не подгрузятся

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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


html
{
background-color: gray;
Copy link

Choose a reason for hiding this comment

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

Вот здесь еще затесалось название цвета


h1
{
white-space: nowrap;
Copy link

Choose a reason for hiding this comment

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

Плохое решение, потому что h1 может занять больше, чем одну строчку. Например
image


h3
{
white-space: nowrap;
Copy link

Choose a reason for hiding this comment

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

Здесь аналогично

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.

3 participants