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

Memory – Pair Game #445

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

Memory – Pair Game #445

wants to merge 2 commits into from

Conversation

okkkko
Copy link
Contributor

@okkkko okkkko commented Jan 29, 2021

Memory – Pair Game

Demo
Code base

The code is submitted in a dedicated feature branch.
Please, review.

@boxdxnx boxdxnx added the Memory Pair Game Task 13: Memory – Pair Game label Feb 1, 2021
@boxdxnx boxdxnx self-assigned this Feb 1, 2021
Copy link
Member

@boxdxnx boxdxnx 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!
Watch my comments below 🐱

@@ -0,0 +1,81 @@
const cardsId = ["01", "02", "03", "04", "05", "06"];
Copy link
Member

Choose a reason for hiding this comment

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

Ids


function createCard(id) {
let flipContainer = document.createElement("div");
flipContainer.className = "flip-container";
Copy link
Member

Choose a reason for hiding this comment

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

classList api is more flexible for styling

let flipContainer = document.createElement("div");
flipContainer.className = "flip-container";
flipContainer.setAttribute("id", id);
gameField.append(flipContainer);
Copy link
Member

Choose a reason for hiding this comment

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

append after all manipulations

CheckPairs();
}
}
function CheckPairs() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not using camelCase here ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably it is a typo:)

cardsArr.forEach((id) => createCard(id));
}
function flipCard(event) {
if (arrOfFlippedCardsId.length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (arrOfFlippedCardsId.length == 0) {
if (!arrOfFlippedCardsId.length) {

firstFlippedContainer = event.target.closest(".flip-container");
firstFlippedContainer.classList.add("clicked");
arrOfFlippedCardsId.push(firstFlippedContainer.id);
} else if (arrOfFlippedCardsId.length == 1 && event.target.closest('.flip-container')!==firstFlippedContainer) {
Copy link
Member

Choose a reason for hiding this comment

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

Use strict equality operator

}
function CheckPairs() {
if (
arrOfFlippedCardsId[0] == arrOfFlippedCardsId[1] &&
Copy link
Member

Choose a reason for hiding this comment

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

use destructuring
example
const [firstId, secondId] =arrOfFlippedCardsId

@boxdxnx boxdxnx added the stale Stale for 15 days or longer label Feb 22, 2021
@OleksiyRudenko
Copy link
Member

@okkkko Please let us know until 20:00 today if you are going to submit any changes into this PR before end of the day February 27. This would help us to plan mentors' limited capacity accordingly.

@okkkko
Copy link
Contributor Author

okkkko commented Feb 26, 2021

Yes, I will, sorry for such a delay:(

@boxdxnx boxdxnx removed the stale Stale for 15 days or longer label Feb 26, 2021
@okkkko okkkko requested a review from boxdxnx February 27, 2021 15:59
@okkkko
Copy link
Contributor Author

okkkko commented Feb 27, 2021

Thanks for the review!

Comment on lines +14 to +34
function createCard(id) {
let flipContainer = document.createElement("div");
flipContainer.classList.add("flip-container");
flipContainer.setAttribute("id", id);
flipContainer.innerHTML = `
<div class="flipper">
<div class="front">
<img src="img/paw.jpg">
</div>
<div class="back">
<img src="img/funny-cat_${id}.jpg">
</div>
</div>`;
gameField.append(flipContainer);
}
function createField() {
cardsArr.sort(function () {
return 0.5 - Math.random();
});
cardsArr.forEach((id) => createCard(id));
}
Copy link
Member

Choose a reason for hiding this comment

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

Adding elements to DOM from a loop is a bad practice. A browser will run reflow and repaint for every element in the loop. Instead, you can: 1. Use append method, which can add several elements in one operation 2. Create some wrapper, add your items to the wrapper and then add it to DOM. It will be one operation. 3. Clone current container. Add items to a container and then replace your old container with a new one. But be aware of event listeners. 4. Use innerHTML instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks ☺️

Comment on lines +36 to +47
if (!arrOfFlippedCardsId.length) {
firstFlippedContainer = event.target.closest(".flip-container");
firstFlippedContainer.classList.add("clicked");
arrOfFlippedCardsId.push(firstFlippedContainer.id);
} else if (
arrOfFlippedCardsId.length === 1 &&
event.target.closest(".flip-container") !== firstFlippedContainer
) {
secondFlippedContainer = event.target.closest(".flip-container");
secondFlippedContainer.classList.add("clicked");
arrOfFlippedCardsId.push(secondFlippedContainer.id);
checkPairs();
Copy link
Member

Choose a reason for hiding this comment

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

DRY

const foo = (flippedContainer) => {
    flippedContainer = event.target.closest(".flip-container");
    flippedContainer.classList.add("clicked");
    arrOfFlippedCardsId.push(flippedContainer.id);
}
...
foo(firstFlippedContainer)

foo(secondFlippedContainer)

@OleksiyRudenko OleksiyRudenko added the stale Stale for 15 days or longer label Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Memory Pair Game Task 13: Memory – Pair Game requires-attention stale Stale for 15 days or longer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants