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

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

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

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

wants to merge 4 commits into from

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

<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 и размеров

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

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

<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?

{
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