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

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

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

Conversation

askeip
Copy link

@askeip askeip commented Nov 7, 2016

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

width: 100%;
}

input

Choose a reason for hiding this comment

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

не обязательно для всех input'ов страницы должно быть display:none

width: 2%;
}

.kisa

Choose a reason for hiding this comment

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

не называй транслитом

padding: 20px;
}

.display-type

Choose a reason for hiding this comment

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

display-type-select

<input name="look" type="radio" value="list" id="list">
<input name="look" type="radio" value="tile" id="tile" checked>
<div class="kisa">
<img src="cat1.jpg" alt="cat1">

Choose a reason for hiding this comment

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

alt должен описывать содержание картинки

</head>
<body>
<div class="display-type">

Choose a reason for hiding this comment

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

section

</div>
<input name="look" type="radio" value="list" id="list">
<input name="look" type="radio" value="tile" id="tile" checked>
<div class="kisa">

Choose a reason for hiding this comment

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

article

@VasiliiKuznecov
Copy link

🍅

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@askeip
Copy link
Author

askeip commented Nov 9, 2016

🍏

@VasiliiKuznecov
Copy link

🚀

@askeip
Copy link
Author

askeip commented Nov 10, 2016

Я так понял,что я зря width и height у img прописывал)

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.

🍅

{
margin: 0 20px;
float: right;
display: inline-block;

Choose a reason for hiding this comment

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

Ты уж реши, тебе float'ами ровнять в строку надо или inline-block'ами?) Совмещать не вижу смысла

border: 5px ridge #d3e2ed;
}

input[id='tile']:checked ~ .cat

Choose a reason for hiding this comment

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

Если используешь id в селекторе, то лучше использовать как #title, чем так. Но у тебя скорее всего линтинг не пройдет, поэтому дай инпутам нормальные классы. И желательно что-бы это был один checkbox состояния, а не два radio

width: 100%;
}

.not-displayed

Choose a reason for hiding this comment

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

Переименуй лучше в hidden

#a9a2ed, 0 0 25px #80ed90;
}

input[id='tile']:checked ~ .cat > :nth-child(n)

Choose a reason for hiding this comment

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

Разъясни пожалуйста, зачем ты тут так делаешь? Почему именно nth-child? Зачем тебе выравнивание text-align: left, зачем такой margin и width?

font-size: .9rem;
}

.cat .kindness img

Choose a reason for hiding this comment

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

Мне не нравится такое решение с выводом звезд =( Можно сделать по другому. Картинки сами визуально норм, но выводить пятью img как-то не очень.

</head>
<body>
<section class="display-type-select">
<label for="list"><img width="512" height="512"

Choose a reason for hiding this comment

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

Во первых - какие-то дюже огромные размеры картинок. Во вторых, эти иконки явно векторные, давай попробуем найти svg. В третьих, выводить их элементами img - не очень =(

<label for="tile"><img width="626" height="626"
src="tile.jpg" alt="Tile view" title="Show cats in tiles"></label>
</section>
<input class="not-displayed" name="look" type="radio" value="list" id="list">

Choose a reason for hiding this comment

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

Поменять класс на hidden, сделать вместо radio один checkbox. Соответственно вместо двух визуальных переключателей нужно сделать один с изменяемым содержимым.

<input class="not-displayed" name="look" type="radio" value="list" id="list">
<input class="not-displayed" name="look" type="radio" value="tile" id="tile" checked>
<article class="cat">
<img src="cat1.jpg" width="700" height="465"
Copy link

@steppefox steppefox Nov 11, 2016

Choose a reason for hiding this comment

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

Очень частый кейс, когда в карточке продукта (товара), картинка тоже кликабельна как и наименование (имя). А еще что-бы при наведении на картинку, имя подчеркивалось, как будто навел на него.

<a class="name" href="http://kupikota.ru/shop.html?i=5702"
title="Высокопородный чистокровный шотландский котик дымн">Высокопородный
чистокровный шотландский котик дымн</a>
<p class="breed">Скоттиш страйт</p>

Choose a reason for hiding this comment

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

В карточках товара, зачастую, категория тоже кликабельна (ссылка).

title="Amount of cats means kindness"></p>
<p class="price">18 000 руб <span class="oldprice">25000 руб</span></p>
</div>
<p class="description">Котенка-котика с отличными породными данными и

Choose a reason for hiding this comment

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

У описания слишком маленький шрифт

@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