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

Develop #1030

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

Develop #1030

wants to merge 15 commits into from

Conversation

IrynaMariiko00
Copy link

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

GJ 👍 Let's improve it a bit)

});

if (game.status === WIN || game.status === LOSE) {
hideMessage();

Choose a reason for hiding this comment

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

You should remove start message:

image

Choose a reason for hiding this comment

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

Also, add margin to Lose message
image

// Затримка для видалення класу 'merged' після завершення анімації
setTimeout(() => {
toCell.classList.remove('merged');
}, 300); // Час анімації (300ms)

Choose a reason for hiding this comment

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

If you change some property in merged class you should add transition for this property in scss file
Now it doesn't work (but not required in the task - so it's only a suggestion)

cell.textContent = '';
} else {
cell.classList.add(`field-cell--${value}`);
cell.textContent = value; // оновлюємо текст

Choose a reason for hiding this comment

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

it's clear without comment 🙂 And it's related to 90% of comments - no need to comment smth if it's clear from the code. If there is complicated logic, unusual behavior, etc. we need to add comments

Copy link
Author

Choose a reason for hiding this comment

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

oh, sorry, I wrote it for myself and forgot to delete it

Iryna Mariiko added 2 commits November 11, 2024 22:50
Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Good job 👍
Let's improve your code
There is no commit from the previous review, you need to commit and push your changes
The text in this button should be centered vertically
image

When we restart the game you need to reset score
image

}
}

// Функція для відображення рекорду

Choose a reason for hiding this comment

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

Remove all comments

Suggested change
// Функція для відображення рекорду

Iryna Mariiko added 2 commits November 12, 2024 09:13
Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Almost done!
This comment still not fixed from the previous review
image

.message-lose {
width: 100%;
box-sizing: border-box;
height: 60px !important;

Choose a reason for hiding this comment

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

Don't use important, it's a bad practice

Suggested change
height: 60px !important;
height: 60px;

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

.

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

good job in general!

@IrynaMariiko00
Copy link
Author

I have improved my code again. Could you look at it, please?

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