Skip to content

Comments

Кужелев Евгений#45

Open
askeip wants to merge 23 commits intourfu-2016:masterfrom
askeip:master
Open

Кужелев Евгений#45
askeip wants to merge 23 commits intourfu-2016:masterfrom
askeip:master

Conversation

@askeip
Copy link

@askeip askeip commented Oct 31, 2016

@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

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@VasiliiKuznecov
Copy link

2016-11-04_00-12-41
В firefox статья слева залезает в центральную колонку

index.css Outdated
margin: 0 1%;
}

thead

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.

image
image

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

Copy link
Author

Choose a reason for hiding this comment

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

теги стоит совсем убрать или селекторы типа .class tag можно использовать?

Choose a reason for hiding this comment

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

такие в общем то можно

index.css Outdated
break-inside: avoid;
}

.col3

Choose a reason for hiding this comment

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

лучше не сокращать

index.css Outdated
font-size: .8rem;
}

.imgblock

Choose a reason for hiding this comment

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

img-block

index.html Outdated
</div>
</header>
<hr>
<div class="main">

Choose a reason for hiding this comment

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

тег main?

index.html Outdated
</header>
<hr>
<div class="main">
<div class="article">

Choose a reason for hiding this comment

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

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

@honest-hrundel
Copy link

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

@askeip
Copy link
Author

askeip commented Nov 4, 2016

Поэкспериментировав со свойством column-fill пришел к выводу, что перенос статьи в другую колонку это лучшее решение в firefox, значении auto становится только хуже

@askeip
Copy link
Author

askeip commented Nov 4, 2016

🍏

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

index.css Outdated
margin: 15px 0;
}

.column-3

Choose a reason for hiding this comment

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

Лучше называть более понятно, например, three-columns-text

index.css Outdated
font-size: 1.5rem;
}

.l

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.

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

Choose a reason for hiding this comment

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

Я читал мангу, смотрел аниме и экранизацию, но название класса осуждаю. Переименовать.

@VasiliiKuznecov
Copy link

🚀

@honest-hrundel
Copy link

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

Copy link

@steppefox steppefox 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: Cloister Black;
src: url(cloisterblack-webfont.woff2)

Choose a reason for hiding this comment

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

Пожалуйста, выровняй format на линии с url к которому он относится. Если тебя беспокоит линтер по поводу запятых, запятые можешь оставить на той же линии где они находятся сейчас. Так же добавь пожалуйста к шрифтам директиву local.

index.css Outdated
html
{
font-size: 16px;
margin: 0 1%;

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.

не понял в чем конкретно проблема здесь. все размеры шрифтов у меня заданы через rem, значит мне нужно задать font-size у html. margin могу перенести в body, без него появляется горизонтальная полоса прокрутки

index.css Outdated

.head
{
width: 100%;

Choose a reason for hiding this comment

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

Элемент header по умолчанию блочный, зачем ему указывать такую ширину?

.head div,
.head h1
{
display: inline-block;

Choose a reason for hiding this comment

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

Зачем делать info и subinfo - display: inline-block, если визуально они выглядят как блочные и им еще руками приходится задавать ширину?

index.css Outdated
.head h1
{
display: inline-block;
margin-bottom: 1%;

Choose a reason for hiding this comment

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

Вертикальные процентные margin очень редкая практика - и это явно не нужно в твоем случае

index.html Outdated
Tim Miller has vacated the director’s chair on Deadpool 2,
it seems that 20th Century Fox is wasting little time in securing a replacement.</p>
<div class="img-block">
<img src="david_leitch.jpg" alt="david leitch">

Choose a reason for hiding this comment

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

alt слишком короткий и написан не корректно (имена пишутся с заглавных букв). Пожалуйста, дай более полное описание картинке. Обязательно нужно выставлять аттрибуты width и height изображениям. А так же заполнять title (можно продублировать из alt)

index.html Outdated
<h3>Death Note (2017 film) info</h3>
<div class="img-block height400">
<img src="l.jpg" alt="l">
<p>I am <span class="l">L</span>, motherfucker</p>

Choose a reason for hiding this comment

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

Плохой класс, переименовать

index.html Outdated
<section>
<h3>Death Note (2017 film) info</h3>
<div class="img-block height400">
<img src="l.jpg" alt="l">

Choose a reason for hiding this comment

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

Опять таки, плохой alt, нужно добавить width, height, title. Исходник картинки больше необходимого по ширине в два раза, нужно выводить исходные изображения того размера, какого они будут в верстке.

Copy link
Author

Choose a reason for hiding this comment

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

Я не понял, что нужно сделать с width и height. Webstorm по умолчанию вставляет размеры изображения в эти параметры, какого размера должна быть картинка в итоге я знать не могу, до того как узнаю ширину колонки. Что от меня требуется?

Choose a reason for hiding this comment

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

Сорри, запутал тебя. Для фиксированных размером картинок width/height обязателен в аттрибутах. Для резиновых нет.

<div class="weather">
<h4>NY Weather</h4>
<p>
SUN OCT 30 Partly cloudy in the morning. Increasing clouds

Choose a reason for hiding this comment

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

Слишком мелкий текст
image

</p>
</div>
<h1>Cinema News</h1>
<div class="rate">

Choose a reason for hiding this comment

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

Цифры и буквы прилипают к границам
image

@honest-hrundel
Copy link

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

@askeip
Copy link
Author

askeip commented Nov 9, 2016

🍏

Copy link

@steppefox steppefox 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

body
{
margin: 0 1%;

Choose a reason for hiding this comment

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

Ну зачем здесь этот несчастный 1%?) Укажи какое-нибудь фиксированное число в px. Кстати текст снизу сильно прижат к краю экрана.

index.html Outdated
<section>
<h3>Death Note (2017 film) info</h3>
<div class="img-block">
<img width="807" height="627" src="l.jpg" alt="Death Note Netflix mem"

Choose a reason for hiding this comment

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

Исходное изображение больше необходимого (даже для экрана 1920px)

@steppefox
Copy link

Еще нужен пример вертикального текста

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

Copy link

@steppefox steppefox 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

.vertical-text
{
float: left;

Choose a reason for hiding this comment

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

Нельзя

index.css Outdated
text-align: center;
text-transform: uppercase;
word-wrap: break-word;
width: 1ch;

Choose a reason for hiding this comment

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

Очень редкая единица размера. Может лучше em?

@steppefox
Copy link

Едет на широком экране
image

@steppefox
Copy link

Also, я не против rem, но в рамках этого задания попрактикуйся пожалуйста с em. Тоесть все rem заменить на em

@honest-hrundel
Copy link

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

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.

4 participants