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
Copy Markdown

@AnastasiaBabushkina AnastasiaBabushkina commented Nov 12, 2016

@honest-hrundel
Copy link
Copy Markdown

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

@honest-hrundel
Copy link
Copy Markdown

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

@honest-hrundel
Copy link
Copy Markdown

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

@AnastasiaBabushkina
Copy link
Copy Markdown
Author

🍏

@dotokoto
Copy link
Copy Markdown

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

Comment thread 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>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

main

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

Choose a reason for hiding this comment

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

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

Comment thread index.html Outdated
<div>
<img class="grandmother" src="картинки/бабушка.png"
alt="Бабушка Галя">
<div class="fstpage">
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 Outdated
<div class="harvest">
<img class="image" tabindex="0" src="картинки/капуста.jpg"
alt="большая капуста">
<span 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.

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

Comment thread index.css Outdated
{
right: 20px;
position: fixed;
height: auto;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread index.css

.image
{
height: auto;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
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
Copy Markdown

🍅

@AnastasiaBabushkina
Copy link
Copy Markdown
Author

AnastasiaBabushkina commented Nov 14, 2016

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

@honest-hrundel
Copy link
Copy Markdown

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

@AnastasiaBabushkina
Copy link
Copy Markdown
Author

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

@dotokoto
Copy link
Copy Markdown

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

@dotokoto
Copy link
Copy Markdown

🚀

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

mokhov commented Nov 21, 2016

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

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

Copy link
Copy Markdown
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.

🍅

Comment thread index.css
body
{
width: 100%;
height: 100%;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Comment thread index.css
height: 100%;
font-family: 'Helvetica Neue', Helvetica, sans-serif;
margin: 20px;
overflow: hidden;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Выглядит как пляска под решение, а если эта галерея будет компонентом на странице, то что?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Comment thread index.css

.image
{
height: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread index.css
input[type='radio'].page3:checked ~ main .firstPage,
input[type='radio'].page3:checked ~ main .secondPage
{
display: none;
Copy link
Copy Markdown
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