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

Козлова Валерия #13

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

Conversation

OnaHochetNaMars
Copy link

@OnaHochetNaMars OnaHochetNaMars commented Nov 28, 2016

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

<div class="content">
<div class="img">
<label for="card-2">
<img src="images/img2.jpg" alt="">
Copy link

Choose a reason for hiding this comment

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

❗️ alt не должен быть пустым. Изображение можно назвать понятней)

<input type="radio" name="gallery" id="card-6" class="card-6">
<input type="radio" name="gallery" id="card-7" class="card-7">
<br>

Copy link

Choose a reason for hiding this comment

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

❗️ Хочется семантичных тегов

<div class="card card-6">
<input type="checkbox" id="show-recipe-6">
<div class="content">
<div class="img">
Copy link

Choose a reason for hiding this comment

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

❗️ Можно придумать более конкретные названия для классов, чем content и img.

<h1>Название рецепта</h1>
<div>
<div class="ingredients">
Ингридиенты
Copy link

Choose a reason for hiding this comment

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

Ингредиенты

</div>
<div class="recipe-description">
<label class="close" for="show-recipe-5">X</label>
<h1>Название рецепта</h1>
Copy link

Choose a reason for hiding this comment

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

❗️ h1 может быть на странице только 1 и должен быть заголовком всей страницы

transition: all .4s ease-in-out;
box-sizing: border-box;
padding-top: 30%;
z-index: -1;
Copy link

Choose a reason for hiding this comment

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

А зачем z-index, если и так opacity: 0?

z-index: 10;
}

body
Copy link

Choose a reason for hiding this comment

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

❗️ Оправдано ли использование position: relative здесь?

z-index: 11;
}

.recipe-description
Copy link

Choose a reason for hiding this comment

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

❗️ Повторяющийся селектор


.recipe-description
{
display: flex;
Copy link

Choose a reason for hiding this comment

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

❗️ Хм, зачем ты используешь flex, если сам крестик направо перемещаешь с помощью text-align?

.ingredients,
.steps
{
width: 50%;
Copy link

Choose a reason for hiding this comment

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

❗️ Если ты используешь флекс у родительского элемента, то располагать элементы внутри лучше с помощью него

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

❗️ По условию задачи, чем дальше фотография от активной - тем меньше должен быть ее поворот. Сейчас у всех неактивных изображений поворот одинаковый
image

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

❗️ Рецепт должен открываться при нажатии на активное изображение, а не только на надпись "РЕЦЕПТ" внутри него

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

❗️ Отсутствует анимация закрытия карточки

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

❗️ Одна карточка превращается в 3. Одна осталась на месте, остальные 2 (с изображением и рецептом) начали переворачиваться, но видно, что они разные по размерам. Необходимо, чтобы визуально карточка всегда была одна
image

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

❗️ Давай карточку рецепта тоже красиво сверстаем, сейчас она выглядит нерепрезентативно. Не хватает отступов, красивого крестика, каких-нибудь разделителей, шрифтов (например, как в макете из условия)

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

В целом здорово, но есть недочеты

@Dodo888
Copy link

Dodo888 commented Nov 30, 2016

🍅

@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

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@OnaHochetNaMars
Copy link
Author

🍏

<input class="show-recipe" type="checkbox" id="show-recipe-5">
<input class="show-recipe" type="checkbox" id="show-recipe-6">
<input class="show-recipe" type="checkbox" id="show-recipe-7">
<div class="card card-1">
Copy link

Choose a reason for hiding this comment

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

❗️ Можно все же использовать семантичные теги main, article, section

<input type="checkbox" id="show-recipe-3">
<div class="content">
<div class="img">
<label for="card-3">
Copy link

Choose a reason for hiding this comment

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

И все-таки?

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

❗️ Иногда при наведении появляется затемнение, но не появляется кнопка "Рецепт"
image

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

❗️ При анимации закрытия карточка мгновенно становится маленькой, потом переворачивается. Уменьшение тоже должно происходить плавно

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

❗️ При открытой карточке должно появляться затемнение, задний фон должен быть неактивным.
image

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

❗️ В Firefox карточка не открывается
image

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

❗️ В Edge при открытой карточке появляется вертикальный скролл
image

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

В Edge проблема со шрифтом (в Chrome все хорошо)
image

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

Карточка теперь выглядит очень классно) Оставил замечания

@Dodo888
Copy link

Dodo888 commented Dec 3, 2016

🚀

@honest-hrundel honest-hrundel assigned msmirnov and unassigned Dodo888 Dec 3, 2016
@msmirnov
Copy link

msmirnov commented Dec 5, 2016

Когда карточка закрывается, то происходят какие-то рывки и анимация совсем не похожа на анимацию открытия.

@msmirnov
Copy link

msmirnov commented Dec 5, 2016

При переключении центральная карточка зачем-то получает hover, хотя я мышку на неё не наводил. Вводит в заблуждение.
screencast 2016-12-05 23-23-23

@msmirnov
Copy link

msmirnov commented Dec 5, 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