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

Трушков Валерий #50

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

Трушков Валерий #50

wants to merge 4 commits into from

Conversation

flafla
Copy link

@flafla flafla commented Nov 7, 2016

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@dotokoto
Copy link

dotokoto commented Nov 9, 2016

  1. В firefox обрезаются верхушки звездочек
    2016-11-09 15-52-46
  2. Нет эффектов при наведении
  3. Нет возможности посмотреть полноразмерную фотографию (этого не требуется по заданию. Но было бы удобно, если бы она была. Можешь добавить, но это не обязательно.)
  4. Если навести на меньший рейтинг, чем уже есть у кота, то звездочки не становятся прозрачными (хотя поставить меньший рейтинг можно, и тогда звездочки станут прозрачными).

<section class="products">
<div class="products-item">
<div class="products-item-image">
<img src="./images/cat1.jpg" alt="котик">
Copy link

Choose a reason for hiding this comment

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

Не хватает title, height и width для картинок

</a>
</div>
<ul class="categories">
<li><a href="#">cat</a></li>
Copy link

Choose a reason for hiding this comment

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

Зачем ссылки?

dicta sequi accusantium!
</div>
</div>

Copy link

Choose a reason for hiding this comment

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

лишняя пустая строка

<input type="checkbox" id="style-switcher" name="style-switcher" value="list" title="list">
<label for="style-switcher">Выводить списком?</label>

<section class="products">
Copy link

Choose a reason for hiding this comment

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

Почему section, а не main?

@dotokoto
Copy link

dotokoto commented Nov 9, 2016

Картинки котиков очень сильно сжимаются при уменьшении ширины экрана
2016-11-09 16-05-08

input[name='style-switcher']:checked ~ .products .products-item-description
{
display: inline-block;
vertical-align: top;
Copy link

Choose a reason for hiding this comment

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

Часто повторяются значения

display: inline-block;
vertical-align: top;

Лучше объединить их в одном блоке, записав селекторы через запятую. А потом отдельно дописывать различающиеся значения

.products-item
{
display: inline-block;
font-size: 1rem;
Copy link

Choose a reason for hiding this comment

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

Почему именно rem?

width: 31%;
}

.products-item .products-item-image img
Copy link

Choose a reason for hiding this comment

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

Может лучше поставить картинкам класс, и использовать селектор по одному этому классу?


.products-item-info .price
{
margin: .31rem 0;
Copy link

Choose a reason for hiding this comment

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

Часто повторяется это значение, надо убрать в один блок

position: absolute;
right: 0;
text-indent: 0;
top: 0;
Copy link

Choose a reason for hiding this comment

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

Лучше top, right, bottom и left записать подряд, а то они чередуются с остальными свойствами

@dotokoto
Copy link

dotokoto commented Nov 9, 2016

🍅

@honest-hrundel
Copy link

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

@dotokoto
Copy link

dotokoto commented Dec 6, 2016

Маловато эффектов при наведении, но в остальном с виду ок

@dotokoto
Copy link

dotokoto commented Dec 6, 2016

🚀

@honest-hrundel honest-hrundel assigned mokhov and unassigned dotokoto Dec 6, 2016
@mokhov
Copy link
Contributor

mokhov commented Dec 10, 2016

– Картинка и ссылка в заголовке должны быть единой ссылкой
– Ссылки должны реагировать на ховер изменением цвета

@mokhov
Copy link
Contributor

mokhov commented Dec 10, 2016

2016-12-10 17-08-55
Очень не нравятся большие дырки, которые появляются на больших экранах

@honest-hrundel
Copy link

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

Copy link
Contributor

@mokhov mokhov left a comment

Choose a reason for hiding this comment

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

🍅

color: #ffdcbd;
}

.rank input:checked + label:hover,
Copy link
Contributor

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.

css-tricks

direction: rtl;
}

.rank legend
Copy link
Contributor

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.

не нужен он тут вовсе. легенда была, но ее не стало, а стили остались


.rank input
{
position: absolute;
Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем нужен position: absolute?

Copy link
Author

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

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

@mokhov
Copy link
Contributor

mokhov commented Dec 13, 2016

2016-12-13 14-19-08
Пока картинки не загружены всё разъезжается

@mokhov
Copy link
Contributor

mokhov commented Dec 13, 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