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

Убогов Сергей #7

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

Conversation

Sergey-Ubogov
Copy link

@Sergey-Ubogov Sergey-Ubogov commented Dec 4, 2016

@honest-hrundel
Copy link

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

@chipolinka
Copy link

chipolinka commented Dec 7, 2016

Привет!
Я смотрю, ты сделал много деталей, характерных для Е1, но при этом не заметил некоторые глобальные вещи:

  • Е1 раскрывается не на весь экран, есть расстояние до краев страницы. А у тебя весь контент от края до края На экранах среднего размера ок)
  • Цвета не совпадают (несложно же такие же подобрать?)
  • Не хватает всё-таки контента справа
  • В оригинале главная новость растянута на весь блок с "все новости", а у тебя -- нет
    default
    Ну а теперь важные мелочи:
  • В шапке сайта есть выпадашка "ещё", которая у тебя не реализована
  • Блок, в котором есть логотип Е1 и всякие шняги типа курса ЦБ, у тебя буквально лежит на новостях, а в оригинале там есть еще расстояние
  • Кажется, у тебя левый верхний угол новости не так оформлен. Вот как надо:
    default
  • В оригинале при наведении на главную новость возникает оранжевая рамка
  • В оригинале между главной новостью и всеми новостями (шапкой "новости") нет расстояния
    default
  • У тебя тут всё поехало в разные стороны, а должно быть по центру между серыми разграничителями. И у погоды с курсом ЦБ должны быть больше расстояния от краёв.
    default
  • Сравни свои шапки блоков новостей с этими. Кажется, у тебя чего-то не хватает
    default
  • Особо хочется увидеть блоки "Видео дня", "Дом" и телепрограмму
  • Было бы круто, если бы ссылки вели на настоящий сайт. К примеру, "Афиша" -- на настоящую Афишу с Е1 :)
  • Ой
    default
  • Поехало
    default

Ой, я тут открыла в Edge, и что увидела...
1.
edge1
2.
edge2
3.
edge3
4.
edge4
А ещё вот Firefox:

  1. Инпуты разной длины
    screenshot from 2016-12-07 23-16-27
  2. Гарантия уехала
    screenshot from 2016-12-07 23-16-46
  3. А здесь я уменьшила экран. Упс)
    screenshot from 2016-12-07 23-33-58

nav,
.top-menu
{

Choose a reason for hiding this comment

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

?

<div><a href="#">Уникальные дома Екатеринбурга</a></div>
<div><a href="#">Споры о Храме-на-воде</a></div>
</div>
<div class="main-header">

Choose a reason for hiding this comment

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

Название надо получше придумать, а то в обычном понимании main и header не могут быть враз

</div>
<div class="border-left">
<div>Воскресенье</div>
<div>04 декабря</div>

Choose a reason for hiding this comment

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

Дату можно в свой тег

<img src="images/e2.jpg" alt="Екатеринбург Он-Лайн" title="Екатеринбург Он-Лайн">
</a>
</div>
<div class="border-left">

Choose a reason for hiding this comment

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

Название не семантично

<div>
<div>Читайте нас где удобно:</div>
<div>
<a href="#"><img src="images/4.jpg" alt="соц. сети" title="соц. сети"></a>

Choose a reason for hiding this comment

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

Соцсети лучше сделать отдельными иконками, каждая -- ссылка

</div>
<div>
<span class="bold">Редакция:</span>
<a href="#">[email protected]</a>

Choose a reason for hiding this comment

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

Для мэйла есть свой тег

<a href="#">Прайс-лист</a>
</div>
<div>
<span class="bold">Единый номер:</span> 8-800-700-8-9-10 (звонок бесплатный)

Choose a reason for hiding this comment

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

И для телефона

title="На трассе Екатеринбург - Пермь
при столкновении фуры и ВАЗа погибли три человека">
</a>
<div class="bold">

Choose a reason for hiding this comment

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

Название класса

@chipolinka
Copy link

Какая у тебя div-ная верстка получилась! Попробуй добавить больше осмысленных подходящих тегов и измени названия классов на более подходящие по смыслу.

@chipolinka
Copy link

🍅

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@chipolinka
Copy link

chipolinka commented Dec 9, 2016

🍅 Найди 2 ошибки
screenshot from 2016-12-09 11-44-24

@honest-hrundel
Copy link

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

@Sergey-Ubogov
Copy link
Author

🍏

@honest-hrundel
Copy link

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

@Sergey-Ubogov
Copy link
Author

🍏

@honest-hrundel
Copy link

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

@Sergey-Ubogov
Copy link
Author

🍏

@chipolinka
Copy link

🚀

@Sergey-Ubogov
Copy link
Author

Эх =(

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