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

Homework2 #2

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

Homework2 #2

wants to merge 4 commits into from

Conversation

zadorotskas
Copy link
Collaborator

No description provided.

@zadorotskas zadorotskas requested a review from aqus October 13, 2021 20:26
<title>CV</title>
<link rel="stylesheet" href="styles.css">
</head>
<body>
Copy link
Member

Choose a reason for hiding this comment

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

Очень не хватает семантической разметки, насколько я помню про нее говорили на лекции. Поисковый робот придя на этот сайт запутается. Также это важно для людей с ограниченными возможностями, читалок и прочей "робототехники", которая будет пытаться понять на машинном уровне структуру сайта.

https://www.freecodecamp.org/news/semantic-html5-elements/

Copy link

Choose a reason for hiding this comment

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

Присоединюсь, было бы все совсем отлично, если бы было немного тегов по смыслу

text-align: center;
}

/* --------- MAIN INFORMATION --------- */
Copy link
Member

Choose a reason for hiding this comment

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

это полезно, коменты в CSS это хорошо, как и везде

@BusinessDuck
Copy link
Member

image

По макету ты правильно сделал размеры контейнеров и общего каркаса, но не правильно спозиционировал аватарку, забыл сделать тень и кнопку "DOWNLOAD"

Проблемы с тенью? Это можно решить! PNG с прозрачностью может отбрасывать тень по контуру без прозрачности

image

Почитай тут https://developer.mozilla.org/en-US/docs/Web/CSS/filter-function/drop-shadow()

@BusinessDuck
Copy link
Member

Я оценил наполнение контентом, но сделал бы его гораздо более компактным, соц сети достаточно только иконки, кажется мы все повязаны с ними на века, и в названиях смысла нет, иконки не очень подобраны для них, все в разной стилистике, нужно найти набор одинаковых иконок соц сетей это можно сделать вот таким запросом:

Copy link

@aqus aqus left a comment

Choose a reason for hiding this comment

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

1 ДЗ - 9 баллов, не хватило семантических тегов
2 ДЗ - 8.5 балла, несоответствие макету, плюс не весь макет воплощен в том виде в котором предлагался

Можно исправлять недочеты и мержить

<title>CV</title>
<link rel="stylesheet" href="styles.css">
</head>
<body>
Copy link

Choose a reason for hiding this comment

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

Присоединюсь, было бы все совсем отлично, если бы было немного тегов по смыслу

styles.css Outdated
p, h2, h3, a {
color: white;
text-decoration: none;
font-family: Helvetica;
Copy link

Choose a reason for hiding this comment

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

есть уже в body

styles.css Outdated
}

p, h2, h3, a {
color: white;
Copy link

Choose a reason for hiding this comment

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

также можно положить в body

styles.css Outdated
margin-right: auto;
}

h2, h3, a {
Copy link

Choose a reason for hiding this comment

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

не очень хорошо стилизовать по селектору тега, лучше стилизовать по селектору класса
Конечно, можно стилизовать таким образом общие для всего документа вещи, но скорее всего мы не захотим видеть ссылки жирными во всем проекте

И еще h2 и h3 намекают нам, что это разные заголовки и они не должны быть одного размера

styles.css Outdated

.projects {
display: flex;
flex-direction: row;
Copy link

Choose a reason for hiding this comment

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

row по умолчанию, убрать тут и везде и везде, где есть

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.

3 participants