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

Рахимянов Эмиль #11

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

Conversation

trszdev
Copy link

@trszdev trszdev commented Oct 21, 2017

@honest-hrundel honest-hrundel changed the title xxxx Рахимянов Эмиль Oct 21, 2017
@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

Copy link

@Mokoshka Mokoshka left a comment

Choose a reason for hiding this comment

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

В Firefox немного не заработало
delorean dmc-12 2017-10-24 15-56-55

index.html Outdated
<body>
<main>
<h1>Lorem Ipsum</h1>
<div class="firstLine">

Choose a reason for hiding this comment

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

Может быть можно использовать что-то более семантичное?

Copy link
Author

Choose a reason for hiding this comment

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

index.html Outdated
</article>
<article>
<h3>Section 1.10.32 of "de Finibus Bonorum et Malorum", written by Cicero in 45 BC</h3>
<div class="vertical">

Choose a reason for hiding this comment

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

И здесь можно использовать более семантичный элемент

index.html Outdated
dolorem eum fugiat quo voluptas nulla pariatur?
</p>
<figure>
<img src="lenna.jpg" alt="lenna.jpg">

Choose a reason for hiding this comment

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

Давай title укажем для картинки

index.css Outdated
word-break: break-all;
border-radius: 5px;
border: 1px solid black;
height: auto;

Choose a reason for hiding this comment

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

Кажется это лишнее

index.css Outdated
width: 1em;
word-break: break-all;
border-radius: 5px;
border: 1px solid black;

Choose a reason for hiding this comment

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

Давай использовать hex-код цветов

index.css Outdated
letter-spacing: .2em;
margin: 10px;
padding: 3px;
font-family: 'Abril Fatface';

Choose a reason for hiding this comment

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

Ты используешь шрифты, которые не подключил. У меня, например, они не отображаются)

index.css Outdated
font-family: 'HFF Clip Hanger';
}

article:nth-child(1) > h3

Choose a reason for hiding this comment

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

Почему бы просто не задать класс, который определяет шрифт? Что-то типа .font-bobo-dylan. Тогда, если ты захочешь еще в каком то элементе использовать шрифт, тебе надо будет всего лишь указать класс нужный.
Предлагаю все селекторы с nth-child переписать таким образом

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

Copy link

@Mokoshka Mokoshka left a comment

Choose a reason for hiding this comment

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

Заголовок в конце колонки выглядит не очень. Давай сделаем так, чтобы заголовок всегда был в одной колонке со своей статьей
delorean dmc-12 2017-11-03 16-26-49

index.css Outdated
border-top: 1px #000 solid;
}

section article:nth-child(2n+1) blockquote
Copy link

Choose a reason for hiding this comment

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

Здесь nth-child оставил. Использование класса значительно упростит селектор и повторное использование таких границ. Например, понадобится использовать такой стиль в другом порядке и тогда надо будет лишь проставить классы у нужных элементов, а не менять селектор. Ведь изменить класс проще?

index.css Outdated
word-break: break-all;
border-radius: 5px;
border: 1px solid #000;
height: auto;
Copy link

Choose a reason for hiding this comment

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

Объясни, пожалуйста, зачем ты здесь используешь height: auto?

index.css Outdated

.vertical
{
display: inline-block;
Copy link

Choose a reason for hiding this comment

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

А зачем этот элемент делать инлайновым?

index.html Outdated
<head>
<meta charset="utf-8">
<title>Задача «DeLorean DMC-12»</title>
<link href="http://tinyurl.com/ycoym3ls" rel="stylesheet">
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Там используются как шрифты из репозитория так и с гугла

Copy link

Choose a reason for hiding this comment

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

Не вижу в этом проблемы. Почему бы просто не подключить те же шрифты?

eyebrow elite added 2 commits November 6, 2017 16:46
@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@honest-hrundel
Copy link

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

@trszdev
Copy link
Author

trszdev commented Nov 6, 2017

Помоги пожалуйста с заголовком в конце колонки. Я пробовал break-after, но он в firefox не работает(( Пробовал article {display:inline-block}, но тогда внутри нет переносов(

@Mokoshka
Copy link

Mokoshka commented Nov 6, 2017

  • Сделай break-after. Это будет верным решением
  • Еще сделай так, чтобы длинный заголовок не переносило
    image
  • Переносов не хватает в тексте, а очень хочется

@honest-hrundel
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants