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

Part3 #2

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

Part3 #2

wants to merge 6 commits into from

Conversation

coradead
Copy link
Collaborator

No description provided.

@coradead coradead requested a review from Leikam November 18, 2021 17:38
Copy link

@Leikam Leikam left a comment

Choose a reason for hiding this comment

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

8 - неплохо. Из минусов - немного компонентов, скорее один общий супер-компонент. нет модификаторов, компоненты не разбиты по файлам и некоторые ошибки в нейминге.

@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link

Choose a reason for hiding this comment

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

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

index.html Outdated
<!DOCTYPE html>
<html lang="en">
<head>
<script>
Copy link

Choose a reason for hiding this comment

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

скрипты лучше выносить в отдельный файл, а те, что подключаются для работы с DOM, либо подключать как defer (отложено) либо в конце файла.
На наших масштабах сейчас это мы не заметим, но в проектах по-больше, при анализе кода, в т.ч. поисковиками, все это учитывается.

index.html Outdated
<head>
<script>
document.addEventListener("DOMContentLoaded", function(event) {
document.querySelector("#switch-theme").addEventListener('click', switchTheme);
Copy link

Choose a reason for hiding this comment

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

все хорошо, хотя в случае если нас ID то есть самый быстрый селектор по дереву, это document.getElementById()

index.html Outdated
});

function switchTheme() {
let bodyCardClassList = document.querySelector("body").classList;
Copy link

Choose a reason for hiding this comment

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

просто document.body – js движок уже сделал быстрые ссылки для популярных элементов

index.html Outdated
function switchTheme() {
let bodyCardClassList = document.querySelector("body").classList;
bodyCardClassList.toggle("dark-theme");
bodyCardClassList.toggle("light-theme");
Copy link

Choose a reason for hiding this comment

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

все правильно: переменная закеширвоана, toggle 👍

index.html Outdated
</div>
<div class="left-part_social-networks">
<a href="https://www.vk.com/coradead">
<img class="left-part_social-networks-img" src="images/vk-logo.png" height="24" alt="vk-logo">
Copy link

Choose a reason for hiding this comment

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

тут скорее MCSS, тк для БЭМ неправильный синтаксис (блок__элемент). А в MCSS стараемся дать представление о вложенности хотя бы на 3 уровнях: left-part_social-networks_img

style.css Outdated
@@ -0,0 +1,122 @@
:root .light-theme {
Copy link

Choose a reason for hiding this comment

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

ок, и рут уже не нужен, раз есть конкретные классы

style.css Outdated
}

body {
background-color: #000000;
Copy link

Choose a reason for hiding this comment

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

не ошибка, но одинаковые пары в hex коды можно сокращать
00000 -> 000
112233 -> 123

в сборке с минификатором CSS это за нас это сделает сборка

font-weight: normal;
}

.left-part_main {
Copy link

Choose a reason for hiding this comment

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

рекомендую также разносить компоненты по своим файлам стилей

</section>
<section class="right-part">
<div class="right-part_main">
<div class="right-part_employee-name">Alex Smith</div>
Copy link

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.

2 participants