Skip to content

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

Open
askeip wants to merge 5 commits intourfu-2016:masterfrom
askeip:master
Open

Кужелев Евгений#29
askeip wants to merge 5 commits intourfu-2016:masterfrom
askeip:master

Conversation

@askeip
Copy link
Copy Markdown

@askeip askeip commented Nov 7, 2016

@honest-hrundel
Copy link
Copy Markdown

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

@honest-hrundel
Copy link
Copy Markdown

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

Comment thread index.css Outdated
width: 100%;
}

input
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.css Outdated
width: 2%;
}

.kisa
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.css Outdated
padding: 20px;
}

.display-type
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

display-type-select

Comment thread index.html Outdated
<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">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.html Outdated
<link rel="stylesheet" type="text/css" href="index.css">
</head>
<body>
<div class="display-type">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

section

Comment thread index.html Outdated
</div>
<input name="look" type="radio" value="list" id="list">
<input name="look" type="radio" value="tile" id="tile" checked>
<div class="kisa">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

article

@VasiliiKuznecov
Copy link
Copy Markdown

🍅

@honest-hrundel
Copy link
Copy Markdown

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

@honest-hrundel
Copy link
Copy Markdown

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

@askeip
Copy link
Copy Markdown
Author

askeip commented Nov 9, 2016

🍏

@VasiliiKuznecov
Copy link
Copy Markdown

🚀

@askeip
Copy link
Copy Markdown
Author

askeip commented Nov 10, 2016

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

Copy link
Copy Markdown

@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.

🍅

Comment thread index.css Outdated
{
margin: 0 20px;
float: right;
display: inline-block;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.css Outdated
border: 5px ridge #d3e2ed;
}

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

Choose a reason for hiding this comment

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

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

Comment thread index.css Outdated
width: 100%;
}

.not-displayed
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.css Outdated
#a9a2ed, 0 0 25px #80ed90;
}

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

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?

Comment thread index.css Outdated
font-size: .9rem;
}

.cat .kindness img
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.html Outdated
</head>
<body>
<section class="display-type-select">
<label for="list"><img width="512" height="512"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.html Outdated
<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">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.html Outdated
<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
Copy Markdown

@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.

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@honest-hrundel
Copy link
Copy Markdown

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

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