-
-
Notifications
You must be signed in to change notification settings - Fork 184
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 from LuckyDnepr #395
Conversation
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
PR marked as "stale", but there is no review. |
UAT done before create PR. Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
Please add files with markup, data, and styles.
const gameField = doc.querySelector(".game_field"); | ||
switch (numberOfCards) { | ||
case 6: | ||
gameField.setAttribute("style", "--cols: 3; --rows: 2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid inline styles, use CSS classes instead.
} | ||
|
||
function checkPair() { | ||
if (openedCards.length % 2 == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (openedCards.length % 2 == 0) { | |
if (openedCards.length === 2) { |
Your array.length never will be greater than 2
Thanks for review!When I look at code from months ago, I want to rewrite everything with new methods, new knowledge. I think this is okay.Result:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress!
<div class="fader hide"></div> | ||
<div class="wrapper"> | ||
<header class="header hide"> | ||
<!-- hide --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
</div> | ||
<div class="nav_menu themes"></div> | ||
<div class="theme_image"> | ||
<img src="" alt="" class="theme_image_img" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set some default value or generate dynamically this block.
</div> | ||
<div class="game_field"></div> | ||
<div class="modal hide"> | ||
<h1 class="win_text">You win!</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From SEO perspective you will have a problem with two h1
headings on one page. try to use one h1
heading per page.
} | ||
|
||
async function readThemes(path) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
const response = await fetch(path); | ||
themesData = await response.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should handle fetch response status - https://www.tjvantoll.com/2015/09/13/fetch-and-errors/
} | ||
|
||
.title { | ||
text-shadow: 3px 3px 5px steelblue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid named colors in CSS, use hex
, rgb(a)
, or hsl
color scheme
width: 100vw; | ||
position: absolute; | ||
background-color: rgba(0, 0, 0, 0.733); | ||
z-index: 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid such a big numbers for z-index
width: 0; | ||
} | ||
|
||
.nav_menu label { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid styling by HTML tags except you add base styles. Reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did I use tag-based styling if I added classes to all these elements? The question is more for me. Practice. More practice.
}); | ||
} | ||
|
||
function generateCardsSet(sizeOfSet, highestNumber) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function generateCardsSet(sizeOfSet, highestNumber) { | |
function generateCardsSet(sizeOfSet, count) { |
function generateCardsSet(sizeOfSet, highestNumber) { | |
function generateCardsSet(sizeOfSet, quantity) { |
function generateCardsSet(sizeOfSet, highestNumber) { | |
function generateCardsSet(sizeOfSet, size) { |
let mixedSetOfPairs = [], | ||
setOfPairs = []; | ||
for (let i = 0; i < sizeOfSet / 2; i++) { | ||
const randomNumber = Math.floor(Math.random() * highestNumber); | ||
if (!setOfPairs.includes(randomNumber)) { | ||
setOfPairs.push(randomNumber, randomNumber); | ||
} else { | ||
i--; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for me you should take a copy of the array with images shuffle it and take first sizeOfSet / 2
elements and then work with this copy. What do you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps.
If I take the first sizeOfSet/2 elements, and then work with them.
I have to somehow get an array of sizeOfSet, in which the indexes of two cards are randomly distributed throughout the sizeOfSet. Again I'll have to shuffle everything.
If I simply glue the two shuffled sizeOfSet/2 element arrays together, I get card[i] === card[sizeOfSet/2 + i], which is too simple gameplay-wise.
I spent a lot of time thinking about randomizing the set of cards, and came to the conclusion that there's no right way to do it. I can generate a jumbled array of 2n cards in different ways.
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
Memory Pair Game
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.