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

Авинас Мария #44

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

Conversation

MaryaAvinas
Copy link

@MaryaAvinas MaryaAvinas commented Nov 7, 2016

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@VasiliiKuznecov
Copy link

насколько понимаю, когда в виде списка, описание кота должно уходить в отдельный столбик

@VasiliiKuznecov
Copy link

при наведении на картинку нет эффекта

@@ -0,0 +1,92 @@
.card-to-list:checked ~ main .catPhoto

Choose a reason for hiding this comment

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

list-view-checkbox

Choose a reason for hiding this comment

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

вместо card-to-list

Choose a reason for hiding this comment

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

не стоит называть классы в camelCase, css не регистрочувствителен
лучше разделять дефисом: cat-photo

width: 100%;
}

header

Choose a reason for hiding this comment

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

Лучше не использовать в селекторе тег, который может быть испольован не один раз на странице

border: 1px solid black;
}

img

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,92 @@
.card-to-list:checked ~ main .catPhoto
{

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.

должно быть:

selector
{
    rules
}

font-size: larger;
}

.image-for-list

Choose a reason for hiding this comment

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

list-icon

<input type="checkbox" class="card-to-list">
<img alt="list" src="spisok.png" title="Список" class="image-for-list">
<main>
<div class="card">

Choose a reason for hiding this comment

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

article

<span>Магазин добрых котиков</span>
</header>
<br>
<input type="checkbox" class="card-to-list">

Choose a reason for hiding this comment

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

лишний отступ

Мяндекс.Муррркет<br>
<span>Магазин добрых котиков</span>
</header>
<br>

Choose a reason for hiding this comment

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

зачем <br>? Если нужен отступ, лучше делать margin-bottom

</header>
<br>
<input type="checkbox" class="card-to-list">
<img alt="list" src="spisok.png" title="Список" class="image-for-list">

Choose a reason for hiding this comment

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

alt

<img alt="list" src="spisok.png" title="Список" class="image-for-list">
<main>
<div class="card">
<img class="catPhoto" alt="котик №1" src="cats_photos/cat1.jpg">

Choose a reason for hiding this comment

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

alt должен описывать содержание картинки, например, "фото полосатого кота"

<span>&#9734;</span>
</div>
<div class="price">
<ins class="current">12 мурлей</ins>

Choose a reason for hiding this comment

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

👍

</div>
<div class="price">
<ins class="current">11 мурлей</ins>
<del class="old">13 мурлей</del>

Choose a reason for hiding this comment

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

лучше назвать old-price и current-price
иначе непонятно будет, когда встретим в css класс old
Или кто-то может создать класс old для отображения другого элемента и это повлияет на это место

@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

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

@MaryaAvinas
Copy link
Author

🍏

width: 100%;
}

header
{
.name-of-market

Choose a reason for hiding this comment

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

market-name, потому что of не несет смысловой нагрузки и лучше записать короче

.card:hover
{
{
background-color: lightgray;

Choose a reason for hiding this comment

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

Лучше не описывать цвет словом, а с помощью hex записи

@VasiliiKuznecov
Copy link

насколько понимаю, когда в виде списка, описание кота должно уходить в отдельный столбик

не увидел изменений

@VasiliiKuznecov
Copy link

🚀

@trixartem
Copy link

trixartem commented Nov 11, 2016

Замечания по верстке:

  • При наведении на элемент все скачет и прыгает
  • Нет внутренних отступов для текста(выглядит не очень красиво)
  • При наведении на карточку фон закрашивается только по верхней границе картинки, что не очень красиво. Сделай, пожалуйста так что бы карточки были все одинаковой высоты.
  • На чекбоксе картинка уехала вверх(смотрел в Яндекс.Браузере) + картинка не кликабельна
  • В целом далеко от основного макета
  • Звездочки не работаю. Было бы здорово добавить эффекты при наведении на звездочки.
  • "Картинки кликабельны" + "Реализовать эффекты при наведении на имя котика, фото, категорию, и плитку в целом" - часть задания, но я этого не увидел:(


.list-view-checkbox:checked ~ main .information .name
{
width: 100%;
Copy link

@trixartem trixartem Nov 14, 2016

Choose a reason for hiding this comment

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

  1. Точно такое же свойство ниже
  2. Для чего ширину менять?


.list-view-checkbox:checked ~ main .card
{
width: 100%;

Choose a reason for hiding this comment

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

Здесь тоже не понятно зачем устанавливать ширину 100%

@trixartem
Copy link

🍅 Жду исправлений

margin: 5px 0;
display: inline-table;
width: 250px;
text-overflow: ellipsed;

Choose a reason for hiding this comment

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

Не правильное свойство. Оно не будет работать.

.card
{
margin: 5px 0;
display: inline-table;

Choose a reason for hiding this comment

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

Почему inline-table?


.card:hover
{
background-color: lightgray;

Choose a reason for hiding this comment

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

Лучше использовать hex запись, что бы запись была короче и этот цвет проще менять(делать светлее темнее и т. д.).

.card:hover
{
background-color: lightgray;
border: 1px solid black;

Choose a reason for hiding this comment

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

#000 короче

<input type="checkbox" class="list-view-checkbox">
<img alt="иконка для изображения списка" src="spisok.png" title="Список" class="list-icon">
<main>
<div class="card">

Choose a reason for hiding this comment

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

Где семантика?

<img class="cat-photo" alt="фото спящего кота" src="cats_photos/cat4.jpg">
<div class="information">
<div class="name">
<a href="#">Кеша</a>

Choose a reason for hiding this comment

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

Лишняя вложенность, кажется что достаточно одной ссылки

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