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

creat branch dev #2

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

creat branch dev #2

wants to merge 18 commits into from

Conversation

nekruz03
Copy link
Collaborator

No description provided.

app.js Outdated
Comment on lines 1 to 2
function loadPageContent(content) {
document.getElementById('content').innerHTML = content;

Choose a reason for hiding this comment

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

Элегантно! Есть ли минусы у такого подхода?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Да, минусы есть. Вставка HTML напрямую может сделать приложение уязвимым к атакам XSS.

index.html Outdated
</section>

<footer>
<p>&copy; 2024 Nazirjonov Nekruz</p>

Choose a reason for hiding this comment

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

Предлагаю добавить динамику.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Добавил

@stepancar
Copy link

На защиту, давайте добавим линтеры и гитхаб экшены. Так же прогу разобраться с вопросом по функции loadPageContent

Copy link

@stepancar stepancar left a comment

Choose a reason for hiding this comment

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

Left some comments

@nekruz03
Copy link
Collaborator Author

Здравствуйте! Если несложно, можете, пожалуйста, выставить балл за эту работу на лабораторную, а вторую визитку которую написал за Работы на паре.

Copy link

@stepancar stepancar left a comment

Choose a reason for hiding this comment

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

PR не готов к ревью, в PR содержится папка node_modules

img/.DS_Store Outdated

Choose a reason for hiding this comment

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

файл не содержит никакого кода

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

удалил

main.js Outdated


function applyNightMode(nightMode) {
document.body.classList.toggle('night-mode', nightMode === 'true');
Copy link

@stepancar stepancar Jan 19, 2024

Choose a reason for hiding this comment

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

обратите внимание, что у вас nightMode может содержать только 2 значение true/false, при этом вы работаете со строками, вам приходится постоянно сравнивать строку с 'true', 'false'.
При этом, причина, по которой вам пришлось так сделать - кроется в в том, что высмешиваете в коде то, как данные хранятся и как они обрабатываются.
Попробуйте разделить это явно. Тогда логика у вас сильно упростится.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

упростил

</section>

<footer>
<p>&copy; 2024 Nazirjonov Nekruz</p>

Choose a reason for hiding this comment

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

добавим динамики?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added dynamics

package.json Outdated
"node": ">=16.17.0"
},
"scripts": {
"lint": "eslint .",

Choose a reason for hiding this comment

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

часто команды объединяют в одну - в которой и eslint линтинг html и css

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

объединил

styles.css Outdated
Comment on lines 146 to 151
.developer-info {
text-align: center;
margin-top: 10px;
}

.developer-info p{

Choose a reason for hiding this comment

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

Код форматирован вручную

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

prettier added

index.html Outdated
<h2>Contact Information</h2>
<div class="contact-details">
<div class="contact-text">
<p>Phone: +7 911 287 82 03</p>

Choose a reason for hiding this comment

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

давайте сделаем открытие стандартного диалого при нажатии

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

добавил

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

сделал

projects.html Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link

@stepancar stepancar left a comment

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