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

Вихарев Вячеслав #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slavavikharev
Copy link

@slavavikharev slavavikharev commented Oct 24, 2017

@honest-hrundel
Copy link

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

Copy link

@VasiliiKuznecov VasiliiKuznecov left a comment

Choose a reason for hiding this comment

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

можно еще добавить куда-нибудь вертикальный текст

display: block;
}

.copyright small:nth-child(2)

Choose a reason for hiding this comment

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

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

text-align: center;
}

.watermark

Choose a reason for hiding this comment

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

watermark это другое, тут же просто author

.quote
{
margin: 0;
font-family: 'Pacifico', cursive;

Choose a reason for hiding this comment

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

попробуй подключить еще один шрифт, но не с гугл-фонтс, а самостоятельно

{
margin: 0;
font-family: 'Pacifico', cursive;
text-indent: 10px;

Choose a reason for hiding this comment

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

тоже наверное лучше указывать в em


.font-size[id='smaller']:checked ~ .newspaper
{
font-size: smaller;

Choose a reason for hiding this comment

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

как мне кажется, лучше использовать конкретные значения вместо ключевых слов

@@ -1,9 +1,100 @@
<!DOCTYPE html>
<html lang="ru">

Choose a reason for hiding this comment

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

кажется, страница уже не на русском

<input class="font-type" type="radio" id="sans-serif" name="font-type">
<label for="sans-serif">Sans-serif</label><br>
<label for="night-mode">Night mode:</label>
<input class="read-mode" type="checkbox" id="night-mode">

Choose a reason for hiding this comment

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

можно все эти настройки объединить в блок с семантичным тегом

<header class="newspaper-header">
<h1 class="newspaper-header-text">The New York Times</h1>
<hr>
<div class="copyright three-columns">

Choose a reason for hiding this comment

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

кажется, название copyright тут не очен подходит, потому что здесь информация не только про это

</div>
</header>
<hr>
<section class="newspaper-section three-columns">

Choose a reason for hiding this comment

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

кажется, здесь article подошел бы больше, ибо это по сути статья

src="http://www.ibiblio.org/eldritch/hmt/myfps2.gif"
alt="Henry Major Tomlinson"
width="307" height="369">
<blockquote class="quote">Donec

Choose a reason for hiding this comment

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

переносы у blockquote в другом стиле, нежели чем у p

@VasiliiKuznecov
Copy link

🍅

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