Skip to content

Бабушкина Анастасия#10

Open
AnastasiaBabushkina wants to merge 4 commits intourfu-2016:masterfrom
AnastasiaBabushkina:master
Open

Бабушкина Анастасия#10
AnastasiaBabushkina wants to merge 4 commits intourfu-2016:masterfrom
AnastasiaBabushkina:master

Conversation

@AnastasiaBabushkina
Copy link

@AnastasiaBabushkina AnastasiaBabushkina commented Nov 12, 2016

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@AnastasiaBabushkina
Copy link
Author

🍏

@dotokoto
Copy link

  1. Картинка прыгает вверх-вправо при наведении на нее. Если медленно подводить курсор с правой стороны картинки, можно получить резкое мигание ( Буде круто, если сможешь сделать, чтобы картинки не прыгали
  2. В firefox не открывается модальное окно с крупной версией
  3. При наведении картинка прилипает к верхней части карточки. Лучше будет выглядеть с отступом сверху
    2016-11-14 15-37-03

index.html Outdated
<input checked name="page" type="radio" class="page1">
<input name="page" type="radio" class="page2">
<input name="page" type="radio" class="page3">
<div>

Choose a reason for hiding this comment

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

main

<input name="page" type="radio" class="page2">
<input name="page" type="radio" class="page3">
<div>
<img class="grandmother" src="картинки/бабушка.png"

Choose a reason for hiding this comment

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

Не хватает title и размеров

index.html Outdated
<div>
<img class="grandmother" src="картинки/бабушка.png"
alt="Бабушка Галя">
<div class="fstpage">

Choose a reason for hiding this comment

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

Непонятное сокращение в названии

index.html Outdated
<div class="harvest">
<img class="image" tabindex="0" src="картинки/капуста.jpg"
alt="большая капуста">
<span class="description">

Choose a reason for hiding this comment

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

Почему именно span, а не p?

index.css Outdated
{
right: 20px;
position: fixed;
height: auto;

Choose a reason for hiding this comment

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

height: auto; точно нужно здесь?


.image
{
height: auto;

Choose a reason for hiding this comment

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

Тот же вопрос про height: auto;

Choose a reason for hiding this comment

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

Вот у картинки height: auto; нужен потому что картинки не квадратные, без него они сильно растягиваются

Copy link
Contributor

Choose a reason for hiding this comment

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

Потому что нужно одно из двух, либо атрибуты, либо размеры в CSS

@dotokoto
Copy link

🍅

@AnastasiaBabushkina
Copy link
Author

AnastasiaBabushkina commented Nov 14, 2016

По поводу firefox, не поняла, у меня открывается

@honest-hrundel
Copy link

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

@AnastasiaBabushkina
Copy link
Author

🍏 исправила замечания. Теперь картинка не прыгает, а увеличивается

@dotokoto
Copy link

Выглядит хорошо ) Будет еще круто, если картинка не будет исчезать с заднего фона при клике на нее

@dotokoto
Copy link

🚀

@honest-hrundel honest-hrundel assigned mokhov and unassigned dotokoto Nov 16, 2016
@mokhov
Copy link
Contributor

mokhov commented Nov 21, 2016

Выглядит хорошо ) Будет еще круто, если картинка не будет исчезать с заднего фона при клике на нее

Не исправлено, в дополнение хочу заметить, что показывается подсказка в полноэкранном режиме. Надо исправить
screen shot 2016-11-21 at 13 29 18 2016-11-21 13-29-56

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.

🍅

body
{
width: 100%;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем нужна 100% высота?

Choose a reason for hiding this comment

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

Позиция у страниц у меня фиксированная, прокрутку я убрала, и для того что бы при наведении на картинку описание отображалось полностью, а не частично, я задаю 100% высоту

height: 100%;
font-family: 'Helvetica Neue', Helvetica, sans-serif;
margin: 20px;
overflow: hidden;
Copy link
Contributor

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.

Не совсем понимаю что не так, margin?


.image
{
height: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Потому что нужно одно из двух, либо атрибуты, либо размеры в CSS

input[type='radio'].page3:checked ~ main .firstPage,
input[type='radio'].page3:checked ~ main .secondPage
{
display: none;
Copy link
Contributor

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