memory-pair-game #138
memory-pair-game #138
Conversation
| class Card { | ||
| constructor(img) { | ||
| this.img = img; | ||
| } | ||
| createCard() { | ||
| return ` | ||
| <li class="card__container"> | ||
| <div class="card card-back"> | ||
| <img src="./img/${this.img}"> | ||
| </div> | ||
| </li>`; | ||
| } | ||
| } |
There was a problem hiding this comment.
No need for this class here. There is no inheritance, no logic inside, etc. Basically, you're using it for data, so just use objects :)
| cards.forEach((card) => (cardsList.innerHTML += card.createCard())); | ||
|
|
||
| const flipCard = ({ target }) => { | ||
| if (target.closest(".card") || target.closest("img")) { |
There was a problem hiding this comment.
Why this ||? This two elements has one parent, right?
| const flipCard = ({ target }) => { | ||
| if (target.closest(".card") || target.closest("img")) { | ||
| clickRemember = [...clickRemember, target.firstElementChild]; | ||
| target.closest(".card").classList.remove("card-back"); |
There was a problem hiding this comment.
Instead of running search in DOM for every card, you can save it into variable inside listener
| @@ -0,0 +1,71 @@ | |||
| const cardsList = document.querySelector(".cards__list"); | |||
| let clickRemember = []; | |||
There was a problem hiding this comment.
Just try to translate this in google translate :) And imagine how it will look like for native speaker :)
| const shuffleCards = (cards) => cards.sort(() => Math.random() - 0.5); | ||
| const createCardOjects = (cards) => cards.map((imgSrc) => new Card(imgSrc)); | ||
| const createAllCards = (cards) => | ||
| cards.forEach((card) => (cardsList.innerHTML += card.createCard())); |
There was a problem hiding this comment.
DOM manipulation in loops.
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:
Use append method, which can add several elements in one operation
Create some wrapper, add your items to the wrapper and then add it to DOM. It will be one operation.
Clone current container. Add items to a container and then replace your old container with a new one. But be aware of event listeners.
Use innerHTML instead
…end-2022-homeworks into memory-pair-game
| cards.forEach((obj) => { | ||
| cardListInnerHtml += createCard(obj, i++); | ||
| }); | ||
| cardsList.innerHTML = cardListInnerHtml; |
There was a problem hiding this comment.
Array#forEach callback has second argument - index
| cardsList.innerHTML = cardListInnerHtml; | ||
| }; | ||
| const shuffleCards = (cards) => cards.sort(() => Math.random() - 0.5); | ||
| let arr = []; |
There was a problem hiding this comment.
Common mistake. Variable name
| clicksStorage[0].id.replace(/\d+/g, "") === | ||
| clicksStorage[1].id.replace(/\d+/g, "") |
There was a problem hiding this comment.
And why you need regexp here? 🤔 I mean, you can just compare ids, without it. No?
| target.closest(".card").classList.add("card-back"); | ||
| target.closest(".card").classList.remove("hide", "open"); |
There was a problem hiding this comment.
Instead of running search on in the loop, you can run it once and save result into variable
There was a problem hiding this comment.
Also, it's same as at line 61-62. So I would move this into separate function - DRY
| </li>`; | ||
| }; | ||
| const flipCard = (target) => { | ||
| if (target.closest(".card")) { |
There was a problem hiding this comment.
I would move this into listener so flipCard will just flip cards. Also, it's a good example of unnecessary nested logic - why do you call function for cases in which it will do nothing? Better not call it all.
| let cardListInnerHtml = ``; | ||
| let i = 0; | ||
| cards.forEach((obj) => { | ||
| cardListInnerHtml += createCard(obj, i++); | ||
| }); | ||
| cardsList.innerHTML = cardListInnerHtml; |
There was a problem hiding this comment.
It will be better to move into separate function. Just for readability
| @@ -0,0 +1,92 @@ | |||
| const cardsList = document.querySelector(".cards__list"); | |||
| let clicksStorage = []; | |||
| let clicksStorage = []; | ||
|
|
||
| const startGame = () => { | ||
| let cards = [ |
There was a problem hiding this comment.
const. Variable is not changed anywhere.
Always use const, except situations in which you will rewrite value in it.
There was a problem hiding this comment.
I cant declare clicksStorage variable with const keyword bcs I change clicksStorage later(69 line)
| return ` | ||
| <li class="card__container"> | ||
| <div class="card card-back"> | ||
| <img src="./img/${src}" id="${imgId}${i}"> |
There was a problem hiding this comment.
Instead of concatination, you can use data-img-id=${imgId}. And then compare by them. And there is no requirement that data-* should be unique
delete .hide from style.css
Memory Pair Game
Demo
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.