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

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

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

Conversation

slavavikharev
Copy link

@slavavikharev slavavikharev commented Nov 8, 2016

@honest-hrundel honest-hrundel changed the title Вихарев Вячесалав Вихарев Вячеслав Nov 8, 2016
@honest-hrundel
Copy link

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

Copy link

@savichev-igor savichev-igor left a comment

Choose a reason for hiding this comment

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

Переключалка неинтересная
image

А тут картинка вообще болеет
image

Выглядит очень раздражающе, когда одинаковый контент прыгает
image

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="13" height="13" viewBox="0 0 13 13"><path fill="#FAB600" d="M6.491 0l1.509 5h5l-4 3 2 5-4.5-3-4.5 3 2-5-4-3h5z"/></svg>

Choose a reason for hiding this comment

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

SVG, найс

Copy link

Choose a reason for hiding this comment

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

Этот svg нигде не используется. Неиспользуемых файлов быть не должно!

.item-category
{
display: inline-block;
color: #666;

Choose a reason for hiding this comment

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

😈


.star-label
{
float: right;

Choose a reason for hiding this comment

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

float нельзя использовать

<div class="card-top">
<img class="item-pic" src="img/1.jpg" alt="1">
</div>
<div class="card-middle">

Choose a reason for hiding this comment

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

Слишком много div'ов, избавляйся от них

@savichev-igor
Copy link

🚀

Copy link

@sladex sladex left a comment

Choose a reason for hiding this comment

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

4, 8 и 9 картинки не отображаются скрин

Странице не хватает главного заголовка (на какую страницу мы попали?).

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="13" height="13" viewBox="0 0 13 13"><path fill="#FAB600" d="M6.491 0l1.509 5h5l-4 3 2 5-4.5-3-4.5 3 2-5-4-3h5z"/></svg>
Copy link

Choose a reason for hiding this comment

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

Этот svg нигде не используется. Неиспользуемых файлов быть не должно!


<section class="item-card">
<div class="card-top">
<img class="item-pic" src="img/1.jpg" alt="1">
Copy link

Choose a reason for hiding this comment

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

"1" - плохой альтернативный текст для картинки

</fieldset>
<div class="item-price">
<h2 class="current-price">£750</h2>
<h3 class="old-price">£800</h3>
Copy link

Choose a reason for hiding this comment

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

Какие же это заголовки?
Тут ты используешь h2/h3 для получения крупного текста. Так делать не нужно - заголовки только для заголовков!

<li class="item-category"><a href="#">British Shorthair</a></li>
<li class="item-category"><a href="#">Chelmsford</a></li>
</ul>
<fieldset class="item-stars">
Copy link

Choose a reason for hiding this comment

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

Кажется ты спутал figure с fieldset :)
http://htmlbook.ru/html/fieldset
http://htmlbook.ru/html/figure

fieldset имеет смысл только внутри форм.

</div>
</div>
<div class="card-bottom">
<p class="item-description">
Copy link

Choose a reason for hiding this comment

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

item-description нет в css. Неиспользуемых классов быть не должно, как и лишних оберток.

2 элемента:
<div class="card-bottom"><p class="item-description">...
тоже самое одним:
<p class="card-bottom">...

<img class="item-pic" src="img/1.jpg" alt="1">
</div>
<div class="card-middle">
<div class="item-info">
Copy link

Choose a reason for hiding this comment

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

item-info нет в css. Неиспользуемых классов быть не должно.

{
display: inline-block;
vertical-align: top;
height: 100%;
Copy link

Choose a reason for hiding this comment

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

У родителя (.item-card) не задана высота, соответственно height: 100% здесь никаким образом на верстку не повлияет.

@sladex
Copy link

sladex commented Nov 14, 2016

🍅

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